From c4d50c97d267cc4130cf21884c992db6129191ee Mon Sep 17 00:00:00 2001 From: Erik Grinaker Date: Sat, 1 Apr 2023 16:52:23 +0000 Subject: [PATCH] kvserver: propose empty entry on unquiescence During Raft unquiescence, an empty command is proposed to wake the Raft leader. However, this empty command was encoded as a regular Raft command with its own command ID and an empty payload. Meanwhile, the downstream Raft application expects such noop commands to be entirely empty (i.e. the entire Raft entry is empty), rather than an entry with an empty payload. This caused it to interpret the command as having a `ProposerLeaseSequence` of 0 and return a `NotLeaseHolderError` rather than considering it a noop. Construction and processing of this error has a non-negligible cost when unquiescing large numbers of ranges. This patch instead proposes an empty entry on unquiescence. Epic: none Release note: None --- pkg/kv/kvserver/kvserverbase/forced_error.go | 10 +++++----- pkg/kv/kvserver/replica_raft_quiesce.go | 4 +--- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/pkg/kv/kvserver/kvserverbase/forced_error.go b/pkg/kv/kvserver/kvserverbase/forced_error.go index 4a4adbbccb0d..5a72bc6dd70a 100644 --- a/pkg/kv/kvserver/kvserverbase/forced_error.go +++ b/pkg/kv/kvserver/kvserverbase/forced_error.go @@ -95,11 +95,11 @@ func CheckForcedErr( requestedLease = *raftCmd.ReplicatedEvalResult.State.Lease } if idKey == "" { - // This is an empty Raft command (which is sent by Raft after elections - // to trigger reproposals or during concurrent configuration changes). - // Nothing to do here except making sure that the corresponding batch - // (which is bogus) doesn't get executed (for it is empty and so - // properties like key range are undefined). + // This is an empty Raft command (which is sent by Raft after elections to + // trigger reproposals, during concurrent configuration changes, or to + // unquiesce the Raft group). Nothing to do here except making sure that the + // corresponding batch (which is bogus) doesn't get executed (for it is + // empty and so properties like key range are undefined). return ForcedErrResult{ LeaseIndex: leaseIndex, Rejection: ProposalRejectionPermanent, diff --git a/pkg/kv/kvserver/replica_raft_quiesce.go b/pkg/kv/kvserver/replica_raft_quiesce.go index 0dd07956fcdb..c7fb66fb9649 100644 --- a/pkg/kv/kvserver/replica_raft_quiesce.go +++ b/pkg/kv/kvserver/replica_raft_quiesce.go @@ -16,7 +16,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv/kvserver/kvserverpb" "github.com/cockroachdb/cockroach/pkg/kv/kvserver/liveness/livenesspb" - "github.com/cockroachdb/cockroach/pkg/kv/kvserver/raftlog" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/util/hlc" "github.com/cockroachdb/cockroach/pkg/util/log" @@ -88,8 +87,7 @@ func (r *Replica) maybeUnquiesceAndWakeLeaderLocked() bool { r.store.unquiescedReplicas.Unlock() r.maybeCampaignOnWakeLocked(ctx) // Propose an empty command which will wake the leader. - data := raftlog.EncodeRaftCommand(raftlog.EntryEncodingStandardWithoutAC, makeIDKey(), nil) - _ = r.mu.internalRaftGroup.Propose(data) + _ = r.mu.internalRaftGroup.Propose(nil) return true }