Use lucene directory abstraction to compute file size #649
Conversation
This file contains 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
vthacker
force-pushed
the
vthacker_NoSuchFileException
branch
from
14eda48 to
530cb6a
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
vthacker
changed the title
vthacker
force-pushed
the
vthacker_NoSuchFileException
branch
from
530cb6a to
0ec848a
Compare
bryanlb
requested changes
Aug 30, 2023
There was a problem hiding this comment.
Looks pretty good - we should update the PR description with content about motivation on this PR.
Let's go ahead and add a test for this as well - if we make the calculateDirectorySize() static we only need to pass in a FSDirectory, which we can use Mockito to stub out the two methods in question (listAll, fileLength)
|
This should help quite a bit on getting more consistent chunk sizes - it looks like by the time we go to close a chunk the size can vary quite a bit. This is the size as seen by the cache node when downloading them: |
vthacker
force-pushed
the
vthacker_NoSuchFileException
branch
from
0ec848a to
26007d3
Compare
bryanlb
approved these changes
Sep 8, 2023
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.
Summary
When we use the regular file abstractions to calculate the size of the lucene index, lucene might be merging segments. This means the function has a race condition between the time it fetches the list and the time the calculation is done.
This is harmless in practice as we just try again and keep the old value. But it leads to noisy logs. When we use Lucene's FS abstraction it has a list of pendingDeletes which helps avoid this race condition.
Another advantage of this PR is during the happy case i.e when the java file abstraction computes the size it can take the pending deletes into account. So the disk size is smaller than what we want to. This will address that problem as well as noted by Bryan