kv: grab raftMu during no-op writes with local gossip triggers #68045
Conversation
|
This change is |
pkg/kv/kvserver/replica_proposal.go
Outdated
| @@ -641,7 +641,9 @@ func addSSTablePreApply( | |||
| return copied | |||
| } | |||
|
|
|||
| func (r *Replica) handleReadWriteLocalEvalResult(ctx context.Context, lResult result.LocalResult) { | |||
| func (r *Replica) handleReadWriteLocalEvalResult( | |||
| ctx context.Context, lResult result.LocalResult, withRaftMu bool, | |||
There was a problem hiding this comment.
withRaftMu doesn't really tell me whether this is the caller asking for raftMu to be acquired, or whether they're promising that they already hold it. Consider calling this raftMuHeld.
| if lResult.MaybeGossipSystemConfig { | ||
| defer maybeAcquireRaftMu()() | ||
| if err := r.MaybeGossipSystemConfigRaftMuLocked(ctx); err != nil { |
There was a problem hiding this comment.
While you're here, sprinkle raftMu.AssertHeld() into the three methods with the RaftMuLocked suffix?
There was a problem hiding this comment.
It's already there. That's the cause of the test flakiness 😃
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @adityamaru and @nvanbenschoten)
|
Looks like this is making it hard to merge stuff, can we land this? I see that CI here is red, so I'm hesitant to pull the trigger. |
Fixes cockroachdb#68011. As of 9f8c019, it is now possible to have no-op writes that do not go through Raft but do set one of the gossip triggers. These gossip triggers require the raftMu to be held, so we were running into trouble when handling the local eval results above Raft. For instance, we see this case when a transaction sets the system config trigger and then performs a delete range over an empty span before committing. In this case, the transaction will have no intents to remove, so it can auto-GC its record during an EndTxn. If its record was never written in the first place, this is a no-op (as of 9f8c019). There appear to be three ways we could solve this: 1. we can avoid setting gossip triggers on transactions that don't perform any writes. 2. we can force EndTxn requests with gossip triggers to go through Raft even if they are otherwise no-ops. 3. we can properly handle gossip triggers on the above Raft local eval result path. This commit opts for the third option.
nvanbenschoten
force-pushed
the
nvanbenschoten/gossipTriggerRaft
branch
from
66b6a88 to
584fb97
Compare
|
bors r+ |
|
Build succeeded: |
Fixes #68011.
As of 9f8c019, it is now possible to have no-op writes that do not go through
Raft but do set one of the gossip triggers. These gossip triggers require the
raftMu to be held, so we were running into trouble when handling the local
eval results above Raft.
For instance, we see this case when a transaction sets the system config
trigger and then performs a delete range over an empty span before
committing. In this case, the transaction will have no intents to
remove, so it can auto-GC its record during an EndTxn. If its record was
never written in the first place, this is a no-op (as of 9f8c019).
There appear to be three ways we could solve this:
any writes.
if they are otherwise no-ops.
path.
This commit opts for the third option.