-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
(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. |
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. |
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. |
sure will revert. new PR coming up |
METADATA
push didn't allow the option ofmoveSegmentToFinalLocation
. 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.