release-21.1: kvserver: synchronize replica removal with read-only requests #64370
Conversation
|
This change is |
erikgrinaker
force-pushed
the
backport21.1-64324
branch
from
c71c5dc
to
d1fbf07
Compare
| @@ -186,8 +186,6 @@ func (r *Replica) destroyRaftMuLocked(ctx context.Context, nextReplicaID roachpb | |||
| // is one, and removes the in-memory raft state. | |||
| func (r *Replica) disconnectReplicationRaftMuLocked(ctx context.Context) { | |||
| r.raftMu.AssertHeld() | |||
| r.readOnlyCmdMu.Lock() | |||
There was a problem hiding this comment.
This is because it's already held? Can we r.readOnlyCmdMu.AssertHeld() in that case (maybe on master only)?
There was a problem hiding this comment.
No, it's not held, but both callers have already called destroyStatus.Set() with it (temporarily) held. As such, the following code paths (including this one) are guaranteed to not conflict with read-only requests since they will either have completed already or will check destroyStatus and abort.
There was a problem hiding this comment.
The point about using r.readOnlyCmdMu.AssertHeld() to assert proper locking is still a good one. Did you give any thought to pulling accesses of r.mu.destroyStatus into methods on the replica and asserting proper handling of r.mu and r.readOnlyCmdMu?
maybe on master only
That shouldn't be needed. AssertHeld()/AssertRHeld() should be no-ops on prod builds. The assertions are only armed in race builds.
There was a problem hiding this comment.
Did you give any thought to pulling accesses of r.mu.destroyStatus into methods on the replica and asserting proper handling of
r.muandr.readOnlyCmdMu?
That's a good idea. I'm going to merge this and the other backports, so we have a fix ready to go, and then submit a follow-up PR to clean up the lock handling a bit. I'd also like to write a quick test for the write path in #46329 first, to see if it has a similar problem.
Replica removal did not synchronize with in-flight read-only requests, which could cause them to be evaluated on a removed (empty) replica, returning an empty result. This patch fixes the problem by locking `Replica.readOnlyCmdMu` during replica removal, thus either waiting for read-only requests to complete or not evaluating them. Release note (bug fix): Fixed a race condition where read-only requests during replica removal (e.g. during range merges or rebalancing) could be evaluated on the removed replica, returning an empty result.
erikgrinaker
force-pushed
the
backport21.1-64324
branch
from
d1fbf07
to
db15a51
Compare
Just the inclusion of #64378 as well, I'll wait for that to be reviewed again and merged before merging the backports. |
| @@ -186,8 +186,6 @@ func (r *Replica) destroyRaftMuLocked(ctx context.Context, nextReplicaID roachpb | |||
| // is one, and removes the in-memory raft state. | |||
| func (r *Replica) disconnectReplicationRaftMuLocked(ctx context.Context) { | |||
| r.raftMu.AssertHeld() | |||
| r.readOnlyCmdMu.Lock() | |||
There was a problem hiding this comment.
The point about using r.readOnlyCmdMu.AssertHeld() to assert proper locking is still a good one. Did you give any thought to pulling accesses of r.mu.destroyStatus into methods on the replica and asserting proper handling of r.mu and r.readOnlyCmdMu?
maybe on master only
That shouldn't be needed. AssertHeld()/AssertRHeld() should be no-ops on prod builds. The assertions are only armed in race builds.
Backport 1/1 commits from #64324.
/cc @cockroachdb/release @cockroachdb/kv
Replica removal did not synchronize with in-flight read-only requests,
which could cause them to be evaluated on a removed (empty) replica,
returning an empty result.
This patch fixes the problem by locking
Replica.readOnlyCmdMuduringreplica removal, thus either waiting for read-only requests to complete
or not evaluating them.
Resolves #64325.
Release note (bug fix): Fixed a race condition where read-only requests
during replica removal (e.g. during range merges or rebalancing) could
be evaluated on the removed replica, returning an empty result.