Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow moveToFinalLocation in METADATA push based on config #8815

Merged
merged 1 commit into from
Jun 2, 2022

Conversation

npawar
Copy link
Contributor

@npawar npawar commented Jun 1, 2022

METADATA push didn't allow the option of moveSegmentToFinalLocation. This meant that if someone had generated segments in a location that was not the deep store, there was absolutely no way to move those segments into deep store without manual scripting.
Chatting with @xiangfu0 , I understood that this was done on purpose, in order to keep METADATA push very light weight and not involve copying from outputDir to dataDir in the push. But this is a very valid scenario where users would want to generate segments in a location other than deep store, and still want to use METADATA push (reasons could be multiple, as listed in #7328).

While there have been more elaborate solutions proposed in the issue above (like periodic background tasks), this PR simply enables one to allow METADATA push to so moveToFinalLocation. And while this would increase the data push time, it is a better solution than asking users to switch to SegmentTarPush or SegmentUriPush if the issue was disparate permissions between outputDir ad dataDir (one of the reasons mentioned in the issue).

Backward compatible behavior or this being false by default is maintained.

@npawar npawar added release-notes Referenced by PRs that need attention when compiling the next release notes Configuration Config changes (addition/deletion/change in behavior) bugfix labels Jun 1, 2022
@npawar npawar requested a review from xiangfu0 June 1, 2022 22:56
@codecov-commenter
Copy link

codecov-commenter commented Jun 1, 2022

Codecov Report

Merging #8815 (6712d64) into master (07b3ee6) will decrease coverage by 0.05%.
The diff coverage is 0.00%.

❗ Current head 6712d64 differs from pull request most recent head c610262. Consider uploading reports for the commit c610262 to get more accurate results

@@             Coverage Diff              @@
##             master    #8815      +/-   ##
============================================
- Coverage     62.93%   62.88%   -0.06%     
- Complexity     4609     4610       +1     
============================================
  Files          1689     1689              
  Lines         89307    89317      +10     
  Branches      13415    13417       +2     
============================================
- Hits          56209    56165      -44     
- Misses        29053    29111      +58     
+ Partials       4045     4041       -4     
Flag Coverage Δ
unittests1 66.23% <0.00%> (+<0.01%) ⬆️
unittests2 14.16% <0.00%> (-0.06%) ⬇️

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

Impacted Files Coverage Δ
...e/pinot/common/utils/FileUploadDownloadClient.java 18.28% <ø> (ø)
...ces/PinotSegmentUploadDownloadRestletResource.java 40.24% <0.00%> (-0.68%) ⬇️
...he/pinot/segment/local/utils/SegmentPushUtils.java 12.50% <0.00%> (-0.21%) ⬇️
...he/pinot/spi/ingestion/batch/spec/PushJobSpec.java 52.00% <0.00%> (-7.10%) ⬇️
...ller/validation/OfflineSegmentIntervalChecker.java 25.84% <0.00%> (-34.84%) ⬇️
...r/validation/RealtimeSegmentValidationManager.java 43.66% <0.00%> (-30.99%) ⬇️
...org/apache/pinot/broker/queryquota/HitCounter.java 92.85% <0.00%> (-7.15%) ⬇️
.../aggregation/function/ModeAggregationFunction.java 87.83% <0.00%> (-0.82%) ⬇️
...core/query/reduce/ExplainPlanDataTableReducer.java 82.75% <0.00%> (-0.69%) ⬇️
...che/pinot/broker/routing/BrokerRoutingManager.java 59.03% <0.00%> (-0.57%) ⬇️
... and 7 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 07b3ee6...c610262. Read the comment docs.

@npawar npawar merged commit 8788f1c into apache:master Jun 2, 2022
@npawar npawar deleted the metadata_push branch June 2, 2022 00:29
@Jackie-Jiang
Copy link
Contributor

(MAJOR) Have we tested this change? I'm pretty sure it won't work as expected. For metadata push, the local segment file only contains the segment metadata, and that is what we'll move to the deep store without the actual segment content.

@npawar
Copy link
Contributor Author

npawar commented Jun 2, 2022

(MAJOR) Have we tested this change? I'm pretty sure it won't work as expected. For metadata push, the local segment file only contains the segment metadata, and that is what we'll move to the deep store without the actual segment content.

hmm you might be right. i tested the move, but missed testing the part of it being metadata only.
Will fix it in a followup PR, and see if there's a test which I can add this flag to

@Jackie-Jiang
Copy link
Contributor

I'd suggest reverting this one for now and add the full support in another PR. For non-metadata push, we upload the segment from local to final location; for metadata push, we need to copy the segment from the download URI to the final location.

@xiangfu0
Copy link
Contributor

xiangfu0 commented Jun 2, 2022

True, this move to the final location should move from the download URI to the final location and then update the download URI in the metadata.

@npawar
Copy link
Contributor Author

npawar commented Jun 2, 2022

I'd suggest reverting this one for now and add the full support in another PR. For non-metadata push, we upload the segment from local to final location; for metadata push, we need to copy the segment from the download URI to the final location.

sure will revert. new PR coming up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Configuration Config changes (addition/deletion/change in behavior) release-notes Referenced by PRs that need attention when compiling the next release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants