storage: remove span declaration for merges #41036
Conversation
|
This change is |
irfansharif
force-pushed
the
merge-spans-cleanup-v2
branch
from
a0ef58a to
10b7910
Compare
There was a problem hiding this comment.
Are there any meaningful implications to this change? Something that comes to mind is that now that we're not latching the LHS rangeID prefix it seems like concurrent lease transfers could proceed. That doesn't seem great as in the face of a re-ordering of commands we might fail out of a merge in the critical phase.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)
There was a problem hiding this comment.
Put another way, would we accept a PR that did the opposite of this to address the case of lease transfers during merges (given merges typically take place with dwindling load)? I'm inclined to think no. I prefer this change if only to be internally consistent, spans are declared across affected keys except when there's a compelling reason to break this rule. But happy to drop this if you disagree, no horses in this race.
Reviewable status:
|
What do you mean by the |
|
Just took a look at #41473 (somehow this flew under my radar). I agree it makes sense latching abort spans during lease transfers, and would play well with this. I'll gate this on #41473 going in if nothing else here seems wonky. edit: err, I have it the other way around. This should go in first for that added latching to make sense. |
Prior to cockroachdb#28661, we used to include a snapshot of the RHS in the merge trigger. Due to this, we declared a read only span over the RHS data range. After we required ranges to be collocated during merges, we didn't need to include the snapshot, so now now longer need to declare the span. Orthogonal to this are Subsume requests that block out concurrent commands to the RHS, with the appropriate span declarations contained therein. Release note: None
irfansharif
force-pushed
the
merge-spans-cleanup-v2
branch
from
10b7910 to
0a24e27
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status:
|
I saw a flaky build getting stuck on |
|
bors r=ajwerner |
41036: storage: remove span declaration for merges r=ajwerner a=irfansharif I'm just re-sending #40261 (reverted in #40446). These changes are intended for 20.1 and will not be merged until after branching. --- Prior to #28661, we used to include a snapshot of the RHS in the merge trigger. Due to this, we declared a read only span over the RHS data range. Now that we require ranges to be collocated during merges, we didn't need to include the snapshot nor declare the span. Orthogonal to this are Subsume requests that block out concurrent commands to the RHS, with the appropriate span declarations contained therein. Release justification: N/A Release note: None Co-authored-by: irfan sharif <[email protected]>
Build succeeded |
I'm just re-sending #40261 (reverted in #40446). These changes are
intended for 20.1 and will not be merged until after branching.
Prior to #28661, we used to include a snapshot of the RHS in the merge
trigger. Due to this, we declared a read only span over the RHS data
range. Now that we require ranges to be collocated during merges, we
didn't need to include the snapshot nor declare the span.
Orthogonal to this are Subsume requests that block out concurrent
commands to the RHS, with the appropriate span declarations contained
therein.
Release justification: N/A
Release note: None