roachpb,storage: Remove the IsNonKV flag #15355
Conversation
|
This change is |
|
Did you Review status: 0 of 3 files reviewed at latest revision, 3 unresolved discussions, some commit checks pending. pkg/roachpb/api.go, line 908 at r1 (raw file):
Can/should we change merge request "declare-keys" to return an empty set? pkg/roachpb/api.go, line 911 at r1 (raw file):
Is it useful to go through the command queue for pkg/roachpb/api.go, line 927 at r1 (raw file):
I'm not clear on why Comments from Reviewable |
|
Huh, well I haven't been able to repro the original failure, but I did hit a different panic: |
|
Reviewed 3 of 3 files at r1. pkg/roachpb/api.go, line 927 at r1 (raw file): Previously, petermattis (Peter Mattis) wrote…
I guess this is what the above comment is trying to justify. Not that it's exactly clear to me. pkg/storage/replica.go, line 2014 at r1 (raw file):
remove this and use pkg/storage/replica.go, line 2033 at r1 (raw file):
remove this nil check? pkg/storage/replica.go, line 2182 at r1 (raw file):
this nil check can go away now. Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed. pkg/storage/replica.go, line 2167 at r1 (raw file):
remove this. Comments from Reviewable |
|
@a-robinson The "span used after call to Finish" is likely a separate. I'd file a new issue and see if @RaduBerinde or @andreimatei can take a look. |
With the move to propEvalKV, the command queue is critical for correct operation and all commands, even non-KV ones, need to go through it. The original motivation for this flag (in cockroachdb#8130) was that non-KV commands were inappropriately synchronizing on the start key of their range; this is no longer true with the move to per-command DeclareKeys functions. This is expected to reduce Merge (timeseries) performance somewhat because it reverts cockroachdb#9889. Fixes cockroachdb#15003
bdarnell
force-pushed
the
remove-nonkv-flag
branch
from
8120323
to
03edc92
Compare
|
I've been seeing "snapshot intersects existing range" errors during |
|
Review status: 2 of 3 files reviewed at latest revision, 7 unresolved discussions, all commit checks successful. pkg/roachpb/api.go, line 908 at r1 (raw file): Previously, petermattis (Peter Mattis) wrote…
No; we still need to use the command queue to manage the merge command's relationship with any concurrent range splits. (we might be able to treat merge like a read instead of a write, but I don't know whether that would actually be safe) pkg/roachpb/api.go, line 927 at r1 (raw file): Previously, tamird (Tamir Duberstein) wrote…
Yeah, that's what the comment is about but I'm having a hard time understanding it through all the layers of history. pkg/storage/replica.go, line 2014 at r1 (raw file): Previously, tamird (Tamir Duberstein) wrote…
Done. pkg/storage/replica.go, line 2033 at r1 (raw file): Previously, tamird (Tamir Duberstein) wrote…
Done. pkg/storage/replica.go, line 2167 at r1 (raw file): Previously, tamird (Tamir Duberstein) wrote…
Done. pkg/storage/replica.go, line 2182 at r1 (raw file): Previously, tamird (Tamir Duberstein) wrote…
Not this one; we assign Comments from Reviewable |
|
@RaduBerinde I don't see why this would lead to "snapshot intersects existing range" issues. |
|
Reviewed 1 of 1 files at r2. Comments from Reviewable |
|
I'm going to merge this so we can start testing it. I still need to think some more about whether there are issues with RequestLease that aren't addressed by this change (and whether the confusing comment mentioned above means we're missing something with TransferLease). |
Restores the pre-cockroachdb#15355 behavior for this command. RequestLease is unique in that it is evaluated on followers too, where the command queue is at best meaningless, and at worst can cause hangs and possibly deadlocks. Fixes cockroachdb#15391
Restores the pre-cockroachdb#15355 behavior for this command. RequestLease is unique in that it is evaluated on followers too, where the command queue is at best meaningless, and at worst can cause hangs and possibly deadlocks. Fixes cockroachdb#15391
|
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful. pkg/roachpb/api.go, line 927 at r1 (raw file): Previously, bdarnell (Ben Darnell) wrote…
FWIW, I think this comment is trying to say that the TransferLeaseRequest can't be checked in Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful. pkg/roachpb/api.go, line 927 at r1 (raw file): Previously, andreimatei (Andrei Matei) wrote…
I agree with the TODO that it would be good to update the comment and point to the mechanism that is protecting Comments from Reviewable |
Restores the pre-cockroachdb#15355 behavior for this command. RequestLease is unique in that it is evaluated on followers too, where the command queue is at best meaningless, and at worst can cause hangs and possibly deadlocks. Fixes cockroachdb#15391
|
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful. pkg/roachpb/api.go, line 927 at r1 (raw file): Previously, petermattis (Peter Mattis) wrote…
Attempting to improve this comment in #15788 Comments from Reviewable |
With the move to propEvalKV, the command queue is critical for correct
operation and all commands, even non-KV ones, need to go through it.
The original motivation for this flag (in #8130) was that non-KV
commands were inappropriately synchronizing on the start key of their
range; this is no longer true with the move to per-command DeclareKeys
functions.
This is expected to reduce Merge (timeseries) performance somewhat
because it reverts #9889.
Fixes #15003