Skip to content

Conversation

@vvivekiyer
Copy link
Contributor

@vvivekiyer vvivekiyer commented Feb 4, 2022

Currently, when a segment is uploaded, we always overwrite if a
segment with the same name already exists. Having an overwrite
parameter in the UploadSegment API will give clients finer
control during uploading segments.

Note that the default value is set to true to retain existing
behavior.

@vvivekiyer vvivekiyer marked this pull request as ready for review February 4, 2022 01:26
@codecov-commenter
Copy link

codecov-commenter commented Feb 4, 2022

Codecov Report

Merging #8125 (4f96c84) into master (8bbf93a) will decrease coverage by 6.65%.
The diff coverage is 28.57%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #8125      +/-   ##
============================================
- Coverage     71.39%   64.74%   -6.66%     
+ Complexity     4303     4302       -1     
============================================
  Files          1624     1579      -45     
  Lines         84198    82322    -1876     
  Branches      12602    12399     -203     
============================================
- Hits          60116    53300    -6816     
- Misses        19970    25228    +5258     
+ Partials       4112     3794     -318     
Flag Coverage Δ
integration1 ?
integration2 ?
unittests1 67.95% <0.00%> (-0.01%) ⬇️
unittests2 14.19% <28.57%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ache/pinot/common/metadata/ZKMetadataProvider.java 20.14% <0.00%> (-59.57%) ⬇️
...e/pinot/common/utils/FileUploadDownloadClient.java 16.35% <0.00%> (-47.32%) ⬇️
...ces/PinotSegmentUploadDownloadRestletResource.java 39.56% <ø> (-18.70%) ⬇️
...ntroller/helix/core/PinotHelixResourceManager.java 61.96% <ø> (-4.04%) ⬇️
...apache/pinot/controller/api/upload/ZKOperator.java 64.56% <100.00%> (-9.84%) ⬇️
...a/org/apache/pinot/common/metrics/MinionMeter.java 0.00% <0.00%> (-100.00%) ⬇️
...g/apache/pinot/common/metrics/ControllerMeter.java 0.00% <0.00%> (-100.00%) ⬇️
.../apache/pinot/common/metrics/BrokerQueryPhase.java 0.00% <0.00%> (-100.00%) ⬇️
.../apache/pinot/common/metrics/MinionQueryPhase.java 0.00% <0.00%> (-100.00%) ⬇️
...ache/pinot/server/access/AccessControlFactory.java 0.00% <0.00%> (-100.00%) ⬇️
... and 372 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 320a5ae...4f96c84. Read the comment docs.

Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overriding a segment is called refresh in pinot. Consider naming the parameter allowRefresh?

@vvivekiyer vvivekiyer changed the title Add overwriteIfExists option to UploadSegment Add allowRefresh option to UploadSegment Feb 7, 2022
@siddharthteotia
Copy link
Contributor

siddharthteotia commented Feb 7, 2022

@kkrugler Please take a look at this PR. Note that our requirement for this is very specific -- for cloud data migration from on-prem where newer segment may have already landed on Azure and the migration script should honor that and not overwrite.

@vvivekiyer
Copy link
Contributor Author

Renamed the parameter to allowRefresh. @Jackie-Jiang please take a look.

Vivek Iyer Vaidyanathan added 3 commits February 7, 2022 17:40
Currently, when a segment is uploaded, we always overwrite if a
segment with the same name already exists. Having an overwrite
parameter in the UploadSegment API will give clients finer
control during uploading segments.

Note that the default option is set to true to retain existing
behavior.
- Improve description for a method.
- Add message when exception is thrown.
@siddharthteotia siddharthteotia merged commit 5ac0daf into apache:master Feb 7, 2022
@vvivekiyer vvivekiyer deleted the overwrite branch August 10, 2022 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants