fix: Prevent over span #3258
fix: Prevent over span #3258
Conversation
fredcarle
added
bug
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3258 +/- ##
===========================================
- Coverage 77.43% 77.39% -0.04%
===========================================
Files 382 381 -1
Lines 35290 35138 -152
===========================================
- Hits 27325 27194 -131
+ Misses 6325 6316 -9
+ Partials 1640 1628 -12
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 17 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Right... I forgot to commit them... |
fredcarle
force-pushed
the
fredcarle/fix/3242-over-span
branch
from
c9bc6ed to
0dd5ccf
Compare
internal/db/fetcher/fetcher.go
Outdated
| ) | ||
| span.Start = span.Start.(keys.DataStoreKey).WithValueFlag() | ||
| // The end key should always be the prefix end of the start key | ||
| span.End = span.Start.PrefixEnd() |
There was a problem hiding this comment.
todo: I think this is incorrect if the user provides two docIDs that are next to each other, there is a function that is called earlier that merges spans that sit next to each other - the change here will mean that the second doc would not be fetched.
e.g.:
User (docIDs: [baefooooooa, baefooooob]{
...
}
We currently have no integration tests for this as it is quite painful to find docs with docIds next to each other, but there are unit tests for the func responsible IIRC.
There was a problem hiding this comment.
we should create a test action testUtils.MineDocWithID :D
There was a problem hiding this comment.
If you're talking about core.MergeAscending, it's called within the same function after this change to the span and should have no impact when querying two adjacent docs.
internal/db/fetcher/fetcher.go
Outdated
| ) | ||
| span.Start = span.Start.(keys.DataStoreKey).WithValueFlag() | ||
| // The end key should always be the prefix end of the start key | ||
| span.End = span.Start.PrefixEnd() |
There was a problem hiding this comment.
we should create a test action testUtils.MineDocWithID :D
| "_docID": "bae-1ef746f8-821e-586f-99b2-4cb1fb9b782f", | ||
| }, | ||
| { | ||
| "_docID": "bae-22dacd35-4560-583a-9a80-8edbf28aa85c", |
There was a problem hiding this comment.
suggestion: use testUtils.NewDocIndex (or whatever the type was)
@AndrewSisley I don't think. there is anything other than what was there before the change that is making the span.End invalid. I initially thought that the problem was somewhere else but the span coming in is never invalid per say. It's not precise but not invalid. What made it invalid was that the function param span.End was already prefix-ended, as it should be, and prefix-ended once more after the instance type being assigned. This means that different path parameters where being prefix-ended and thus causing an over span. |
We can fix this then, and prefix-end the correct key property, it shouldn't be terribly difficult, and then we wont be introducing a new bug/ignoring the span.end |
It is fixed with this change. As stated above, there is no new bug I believe. |
|
@fredcarle I had a bit of a think, and chased down the references to
However:
Technically I think the best thing to do is to remove the I suggest you merge the fix as-is, and then I carry out the proposed refactor. If you disagree with the proposed refactor, I remain against the proposed solution for the reasons in (1). Let me know 😁 Sorry if this feels a little hostage-like 😅 |
I don't disagree.
This is what I thought of doing at some point but I wanted a quick fix first because I wasn't sure how deep of a rabbit hole I would be getting into. I should have documented that in the PR description. I'm keen on trying to do the "spans to array of prefix" change in this PR. Unless you prefer taking that on. |
fredcarle
force-pushed
the
fredcarle/fix/3242-over-span
branch
from
0dd5ccf to
471ab9c
Compare
Relevant issue(s)
Resolves #3242
Description
This PR solves the situation where deleted documents in the immediate next collection by ID were returned on a full collection query. The reason for this behaviour was due to the fetcher start method redefining the spans based on wanting deleted docs or not and was defining an end key that might have been "prefix-ended" based on a shorter prefix (i.e. collection instead of instance type).
The solution was to always redefine the end key as the prefix end of the start key.To fix this we removed the concept of spans and replaced it with a list of prefixes. This results in the fetcher being asked, for example, for all docs in collection 1 with a prefix of
/data/1instead of a span from/data/1to/data/2. Furthermore, when the fetcher checks if it need to get deleted docs or non-deleted docs, the resulting prefix becomes/data/1/< v or d >instead of the span from/data/1/< v or d >to/data/2/< v or d >(the span is wrong).The first commit documents the bug with an integration test.
Tasks
How has this been tested?
make test
Specify the platform(s) on which this was tested: