storage: remove span declaration for merges#40261
storage: remove span declaration for merges#40261craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
f1e52bc to
31e4f48
Compare
|
(Friendly ping.) |
ajwerner
left a comment
There was a problem hiding this comment.
Mostly comment nits.
It seems like this change makes merges less likely to block on pending writes. This change doesn't actually make merges less disruptive to other writes, right? If anything it seems like this change may lead to extra work for commands destined to fail after the merge applies. Any command which is evaluated prior to the merge before this change would lead to the merge blocking on its latches so that will be relieved. However, if I read this correctly, any command which will now be free to evaluate after the merge (and isn't reordered before it) will be destined to fail to apply and thus we'll end up replicating commands for no good reason.
Reviewed 2 of 3 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @irfansharif, and @nvanbenschoten)
pkg/roachpb/data.go, line 173 at r1 (raw file):
// PrefixEnd determines the end key given key as a prefix, that is the // key that sorts precisely behind all keys starting with prefix: "1" // is added to the final byte and then carry propagated. The special
I think this was more correct before.
pkg/storage/batcheval/cmd_end_transaction.go, line 127 at r1 (raw file):
} if mt := et.InternalCommitTrigger.MergeTrigger; mt != nil { // Merges only copy over the RHS abort span to the LHS, and computes
nit: s/computes/compute/ there's subject-verb number problem here with computes.
super nit: only doesn't add much meaning to me here.
pkg/storage/batcheval/cmd_end_transaction.go, line 1027 at r1 (raw file):
// mergeTrigger is called on a successful commit of an AdminMerge transaction. // It recomputes stats for the LHS by merging in RHS stats, and copies over the
nit: calculates to me is clearer than recomputes which is the term we use to mean scanning the data to compute the stats.
|
This "destined to fail" comment I made now seems wrong. Rather, it's not clear how a write outside of the LHS's prior key range could be evaluated until the merge has been applied. I suppose there's an instant after which the merge has been applied before it has been acknowleged during which we'll not allow a proposal to be evaluated but by the time the updated range descriptor is visible to a request all durable work to the engine is done. I think I'm convinced that my previous concerns don't 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. 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 note: None
31e4f48 to
0e9bb0c
Compare
irfansharif
left a comment
There was a problem hiding this comment.
Resolved comment nits.
Reviewable status:
ajwerner
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2.
Reviewable status:
|
bors r+ |
40261: storage: remove span declaration for merges r=irfansharif a=irfansharif 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. Also informs #32583. Orthogonal to this are Subsume requests that block out concurrent commands to the RHS, with the appropriate span declarations contained therein. Release note: None Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>
Build succeeded |
|
Sorry about letting this review sit @irfansharif. The change LGTM based on my understanding of the synchronization before and after a merge trigger is applied, but I don't think we want to risk it for the next release given that we're in the stability period and there's not a pressing need to get this in. Do you mind reverting it temporarily until the release branch is cut and master opens back up for risky changes? |
Reverts cockroachdb#40261. Holding off until release branch is cut. Release note: None
|
Sure, #40446. |
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 <irfanmahmoudsharif@gmail.com>
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.
Also informs #32583.
Orthogonal to this are Subsume requests that block out concurrent
commands to the RHS, with the appropriate span declarations contained
therein.
Release note: None