kv: stop DistSender from double-covering RangeFeeds during splits #35466
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
|
This change is |
nvanbenschoten
approved these changes
Mar 11, 2019
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @nvanbenschoten and @tbg)
If a RangeFeed was running over `[/a,/d)` and it split at `/b`, then we'd get an error from the server and the span would be kicked out to `divideAndSendRangeFeedToRanges`. The `RangeIterator` would usually hand out the post-split `[/a,/b)` descriptor for `/a`, then advance to `/b` and first try the pre-split `[/a,/d)` descriptor. Each would get a new RangeFeed. The one over `[/a,/d)` would immediately come back with an error from the server that it didn't fit in the bounds of a range, evict the `RangeDescriptorCache` token, and get kicked out again to `divideAndSendRangeFeedToRanges`. This time, because of the eviction, the `RangeIterator` would get the post-split `[/b,/d)` descriptor. The end result was one RangeFeed over `[/a,/b)` and two over `[/b,/d)`. A second split at `/c` meant we could double it again and end up with 4 over `[/c,/d)`. RangeFeed always has the potential for sending duplicates, so changefeeds have to be resilient to this, and this was not a correctness issue, but it's obviously bad. The fix is simple: use the same 'nextRS' trick that `divideAndSendBatchToRanges` does to keep track of the uncovered part of the input `rs` span to `divideAndSendRangeFeedToRanges` and use that to trim the descriptors that come back. Found when splits were added to TestChangefeedNemeses and manually investiating why RangeFeed was returning duplicates. No test yet since one is coming in cockroachdb#32721. Release note: None
danhhz
force-pushed
the
rangefeed_distsender
branch
from
0c3e267 to
e0f66ed
Compare
|
Flake looks like #35550. Thanks for the review! bors r=nvanbenschoten |
Build failed (retrying...) |
craig bot
pushed a commit
that referenced
this pull request
Mar 11, 2019
35466: kv: stop DistSender from double-covering RangeFeeds during splits r=nvanbenschoten a=danhhz If a RangeFeed was running over `[/a,/d)` and it split at `/b`, then we'd get an error from the server and the span would be kicked out to `divideAndSendRangeFeedToRanges`. The `RangeIterator` would usually hand out the post-split `[/a,/b)` descriptor for `/a`, then advance to `/b` and first try the pre-split `[/a,/d)` descriptor. Each would get a new RangeFeed. The one over `[/a,/d)` would immediately come back with an error from the server that it didn't fit in the bounds of a range, evict the `RangeDescriptorCache` token, and get kicked out again to `divideAndSendRangeFeedToRanges`. This time, because of the eviction, the `RangeIterator` would get the post-split `[/b,/d)` descriptor. The end result was one RangeFeed over `[/a,/b)` and two over `[/b,/d)`. A second split at `/c` meant we could double it again and end up with 4 over `[/c,/d)`. RangeFeed always has the potential for sending duplicates, so changefeeds have to be resilient to this, and this was not a correctness issue, but it's obviously bad. The fix is simple: use the same 'nextRS' trick that `divideAndSendBatchToRanges` does to keep track of the uncovered part of the input `rs` span to `divideAndSendRangeFeedToRanges` and use that to trim the descriptors that come back. Found when splits were added to TestChangefeedNemeses and manually investiating why RangeFeed was returning duplicates. No test yet since one is coming in #32721. Release note: None Co-authored-by: Daniel Harrison <[email protected]>
Build succeeded |
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.
If a RangeFeed was running over
[/a,/d)and it split at/b, thenwe'd get an error from the server and the span would be kicked out to
divideAndSendRangeFeedToRanges. TheRangeIteratorwould usually handout the post-split
[/a,/b)descriptor for/a, then advance to/band first try the pre-split
[/a,/d)descriptor. Each would get a newRangeFeed. The one over
[/a,/d)would immediately come back with anerror from the server that it didn't fit in the bounds of a range, evict
the
RangeDescriptorCachetoken, and get kicked out again todivideAndSendRangeFeedToRanges. This time, because of the eviction,the
RangeIteratorwould get the post-split[/b,/d)descriptor. Theend result was one RangeFeed over
[/a,/b)and two over[/b,/d). Asecond split at
/cmeant we could double it again and end up with 4over
[/c,/d). RangeFeed always has the potential for sendingduplicates, so changefeeds have to be resilient to this, and this was
not a correctness issue, but it's obviously bad.
The fix is simple: use the same 'nextRS' trick that
divideAndSendBatchToRangesdoes to keep track of the uncovered part ofthe input
rsspan todivideAndSendRangeFeedToRangesand use that totrim the descriptors that come back.
Found when splits were added to TestChangefeedNemeses and manually
investiating why RangeFeed was returning duplicates. No test yet since
one is coming in #32721.
Release note: None