download and write by fetch info instead of by term #256
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
assafvayner
force-pushed
the
assaf/download-write-by-fetch-info
branch
from
2f83f2c to
ad78a4e
Compare
…nload-write-by-fetch-info
seanses
reviewed
Apr 23, 2025
seanses
reviewed
Apr 23, 2025
seanses
reviewed
Apr 23, 2025
seanses
reviewed
Apr 23, 2025
seanses
reviewed
Apr 23, 2025
seanses
reviewed
Apr 23, 2025
seanses
reviewed
Apr 23, 2025
seanses
reviewed
Apr 23, 2025
seanses
reviewed
Apr 23, 2025
seanses
reviewed
Apr 23, 2025
| terms, | ||
| offset_into_first_range, | ||
| segment_size, | ||
| total_len, |
There was a problem hiding this comment.
BUG: this parameter is called "remaining_total_len" but this never gets updated, i.e. ranges will be written at incorrect offset for 2+ segments.
There was a problem hiding this comment.
this turns out to need to be passed in total_len in this case, the calculation ensures not writing past the end of the requested range.
There was an additional bug here though that each segment began writing at byte 0.
There was a problem hiding this comment.
If you derive the remaining_total_len correctly, then the start offset for each segment is total_len - remaining_total_len
seanses
reviewed
Apr 23, 2025
seanses
requested changes
Apr 23, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix XET-493.
This change changes the standard parallelized download process to run with respect to fetch info's within a segment rather than on a term by term basis.
Why? terms which are adjacent or overlapping in a xorb utilize a single fetch info instance or a presigned url to CDN/S3. Downloading term by term can cause repeat downloads of the same fetch info url. We do not often see this happen because the cache & single flight are used to combat this happening. However the cache does not help if it is disabled/slow, and in general we can get rid of the latency even for a cache hit (file read into memory) with this change. Notably local cache hit is still better than a CDN download so we still check the cache first.