server: simplify SplitRangeWithExpiration#64155
server: simplify SplitRangeWithExpiration#64155craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
erikgrinaker
left a comment
There was a problem hiding this comment.
Nice simplification!
Reviewed 1 of 1 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @tbg)
pkg/server/testserver.go, line 1190 at r1 (raw file):
return err } leftRangeDesc = more[0]
Is there a case where more is an empty slice, e.g. if the split has since been merged and the key now resides in the first range?
When executing a split, it's surprisingly tricky to learn what the resulting left-hand and right-hand side is. This is because when you retrieve it "after the fact", other operations may have changed the split already (for example, the split could have been merged, or additional splits added) and while you would get descriptors back, they wouldn't be meaningfully connected to the split any more in all cases. Really one would want to return the descriptors from the split txn itself, but AdminSplit is a no-op when the split already exists and so we would need special logic that performs a range lookup on the left neighbor. It could all be done, but does not seem worth it. There's still a nice simplification here that lets us remove the ad-hoc code, and we'll just accept that when there are concurrent splits the return values may not exactly line up with the split. This came out of cockroachdb#64060 (comment). Release note: None
tbg
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/server/testserver.go, line 1190 at r1 (raw file):
Previously, erikgrinaker (Erik Grinaker) wrote…
Is there a case where
moreis an empty slice, e.g. if the split has since been merged and the key now resides in the first range?
Yep, this could happen in theory. I changed the code to throw errors whenever there aren't two ranges to be returned. Thanks for pointing this out.
erikgrinaker
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status:
|
bors r=erikgrinaker |
|
Build failed (retrying...): |
|
Build succeeded: |
When executing a split, it's surprisingly tricky to learn what the resulting
left-hand and right-hand side is. This is because when you retrieve it "after
the fact", other operations may have changed the split already (for example,
the split could have been merged, or additional splits added) and while you
would get descriptors back, they wouldn't be meaningfully connected to the
split any more in all cases.
Really one would want to return the descriptors from the split txn itself, but
AdminSplit is a no-op when the split already exists and so we would need
special logic that performs a range lookup on the left neighbor. It could all
be done, but does not seem worth it. There's still a nice simplification here
that lets us remove the ad-hoc code, and we'll just accept that when there are
concurrent splits the return values may not exactly line up with the split.
This came out of #64060 (comment).
cc @cockroachdb/kv
Release note: None