Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upsegment/findEntry: do not consider offset 0 as an invalid read value #142
Conversation
66591a8
to
7b5be6a
Codecov Report
@@ Coverage Diff @@
## master #142 +/- ##
==========================================
+ Coverage 41.4% 41.44% +0.04%
==========================================
Files 84 84
Lines 5936 5935 -1
==========================================
+ Hits 2458 2460 +2
+ Misses 3022 3020 -2
+ Partials 456 455 -1
Continue to review full report at Codecov.
|
|
@jbonachera thanks for this. I was also thinking about just initializing the segment with -1 to know for sure if the segment is empty or not. |
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.
When looking for an entry in a segment based on its offset,
findEntryseems to consider that the entry does not exist if the read offset is 0.However, as the entry used in the
sort.Searchtest is reused across all search iterations (it's a pointer), the entry offset may be greater than 0 even if the found entry does not exist.I think we should not rely on entry's offset value if
ReadEntryAtFileOffsetreturned an error.(I might be of course wrong, so i'm open to suggestions or explanations.)
Details
Here's an example of the scenario addressed by this PR (I've implemented it in a test):
We have 12 messages in the log, and we are looking for all messages from the offset n° 11. We should get 1 message (offset 11)
sort.Searchwill start by looking for the first value in the index that do not satisfy the test, starting withn, then dividing it by two until the test returns false; so we have (I got this output by adding somefmt.Printfin the code) :sort.Searchfound that offset 10 is the first offset not satisfing the test.We now known that the wanted value (which is the first value to satisfy the test) is in the interval
[10, 20)(because 20 satisfied the test).sort.Searchwill now try to find this value, by reducing this interval:Because the Entry inside the
sort.Searchhad been filled with entry from offset 10, and not re-initialized since, it does not passed the test used bysort.Search.sort.Searchthus returns offset 20 as result, instead of offset 11.The last
ReadEntryAtFileOffsetthen silently fails, and the returned offset is 10.