kvserver: clean up replica unquiescence #105041

erikgrinaker · 2023-06-16T12:37:58Z

kvserver: remove stale maybeQuiesce TODO

The leader won't quiesce if followers aren't caught up.

kvserver: remove maybeUnquiesceAndWakeLeaderLocked

This patch merges maybeUnquiesceAndWakeLeaderLocked() into maybeUnquiesceWithOptionsLocked(), using a separate parameter to wake the leader. Care is taken to make this purely mechanical, with no logical changes at all.

kvserver: remove maybeUnquiesceWithOptionsLocked

This patch merges maybeUnquiesceWithOptionsLocked() into maybeUnquiesceLocked(), requiring callers to always specify options. The subtlety around unquiescence, and few call sites, makes it beneficial to be explicit.

This is a purely mechanical change, with no logical changes. A couple of tests have been changed to now wake the leader when unquiescing, but this has no bearing on the tests.

kvserver: set last update times on leader unquiesce

Previously, Replica.lastUpdateTimes was updated whenever a replica unquiesced without attempting to wake the leader. However, this had two flaws: a leader could fail to call it if it hit a code path where it did attempt to wake the leader (even if it was leader itself), e.g. by returning true from withRaftGroup(), and it could also be called on a follower where it would have no effect.

This patch instead updates it when unquiescing the leader, regardless of unquiesce options.

kvserver: only wake leader when unquiescing a follower

Previously, any replica would wake the leader when unquiescing, if requested by the caller. However, this could cause the leader to propose an empty command to wake itself, which commonly happens in handleRaftReady() via withRaftGroupLocked(). This appears unnecessary, and likely causes a large amount of Raft proposals with ranges that frequently (un)quiesce.

This patch instead only attempts to wake the leader from followers.

kvserver: wake leader before campaigning when unquiescing

We should wake the leader before campaigning when unquiescing, since we won't send the proposal in the candidate state and thus won't give the leader a chance to assert leadership if we're wrong about it being dead.

kvserver: omit unnecessary campaign checks on leader unquiescence

This patch changes a couple of maybeUnquiesce() call sites to not attempt to campaign when we know the replica must already be the leader.

Epic: none
Release note: None

cockroach-teamcity · 2023-06-16T12:38:14Z

This change is

erikgrinaker · 2023-06-16T12:39:41Z

@tbg @pavelkalinnikov Would like some scrutiny of the "only wake leader when unquiescing a follower" commit here. It seems problematic and unnecessary that we submit a proposal when the leader wakes itself, but it's possible that we've come to rely implicitly on this logic.

~~I expect some test fallout from this as well.~~ Appears to pass. Maybe this is fine.

erikgrinaker · 2023-06-16T14:56:28Z

This resulted in a substantial improvement when running kv0 on 20k ranges with COCKROACH_QUIESCE_AFTER_TICKS=0 where we aggressively quiesce ranges (the default in 23.1). The Raft proposal rate was cut almost by half, and the Raft message rate dropped by 30%:

Throughtput improved by 6% (sample size of 1 though):

_elapsed___errors_____ops(total)___ops/sec(cum)__avg(ms)__p50(ms)__p95(ms)__p99(ms)_pMax(ms)__total
  180.0s        0        1840221        10223.4     18.8     15.2     50.3     71.3    520.1  write

_elapsed___errors_____ops(total)___ops/sec(cum)__avg(ms)__p50(ms)__p95(ms)__p99(ms)_pMax(ms)__total
  180.0s        0        1953950        10855.3     17.7     13.6     52.4     71.3    469.8  write

I'm not sure that this is safe yet, since I saw a fair bit of election timeouts after cold starting the cluster. Having a closer look.

erikgrinaker · 2023-06-16T15:06:33Z

I'm not sure that this is safe yet, since I saw a fair bit of election timeouts after cold starting the cluster.

Saw the same problem with master, so it isn't this PR, but we may have broken something recently. Digging in.

erikgrinaker · 2023-06-16T15:45:00Z

we may have broken something recently.

#105052

tbg

Nice, thanks.

The leader won't quiesce if followers aren't caught up. Epic: none Release note: None

This patch merges `maybeUnquiesceAndWakeLeaderLocked()` into `maybeUnquiesceWithOptionsLocked()`, using a separate parameter to wake the leader. Care is taken to make this purely mechanical, with no logical changes at all. Epic: none Release note: None

This patch merges `maybeUnquiesceWithOptionsLocked()` into `maybeUnquiesceLocked()`, requiring callers to always specify options. The subtlety around unquiescence, and few call sites, makes it beneficial to be explicit. This is a purely mechanical change, with no logical changes. A couple of tests have been changed to now wake the leader when unquiescing, but this has no bearing on the tests. Epic: none Release note: None

Previously, `Replica.lastUpdateTimes` was updated whenever a replica unquiesced without attempting to wake the leader. However, this had two flaws: a leader could fail to call it if it hit a code path where it did attempt to wake the leader (even if it was leader itself), e.g. by returning `true` from `withRaftGroup()`, and it could also be called on a follower where it would have no effect. This patch instead updates it when unquiescing the leader, regardless of unquiesce options. Epic: none Release note: None

Previously, any replica would wake the leader when unquiescing, if requested by the caller. However, this could cause the leader to propose an empty command to wake itself, which commonly happens in `handleRaftReady()` via `withRaftGroupLocked()`. This appears unnecessary, and likely causes a large amount of Raft proposals with ranges that frequently (un)quiesce. This patch instead only attempts to wake the leader from followers. Epic: none Release note: None

We should wake the leader before campaigning when unquiescing, since we won't send the proposal in the candidate state and thus won't give the leader a chance to assert leadership if we're wrong about it being dead. Epic: none Release note: None

This patch changes a couple of `maybeUnquiesce()` call sites to not attempt to campaign when we know the replica must already be the leader. Epic: none Release note: None

pav-kv · 2023-06-20T09:31:14Z

pkg/kv/kvserver/replica_raft_quiesce.go

+	if wakeLeader {
+		// Propose an empty command which will wake the leader.
+		if log.V(3) {
+			log.Infof(ctx, "waking %d leader", r.RangeID)


How about "r%d" instead of "%d". It's easier to search by "rNNN" in logs when investigating, this avoids false positives.

I was following the convention of the existing log messages here, didn't particularly feel like updating a bunch of log messages. But I can do a pass if there aren't that many to update.

Tacked on a commit.

pav-kv · 2023-06-20T09:41:58Z

pkg/kv/kvserver/helpers_test.go

@@ -495,15 +495,7 @@ func (r *Replica) GetQueueLastProcessed(ctx context.Context, queue string) (hlc.
 }

 func (r *Replica) MaybeUnquiesce() bool {


The semantics of MaybeUnquiesce changed. Were there any callers who wanted the old behaviour (wakeLeader=mayCampaign=false)?

The tests don't care -- explained in the commit message:

A couple of tests have been changed to now wake the leader when unquiescing, but this has no bearing on the tests.

In case it wasn't clear, this is a test-only file, helpers_test.go.

pav-kv · 2023-06-20T09:49:12Z

pkg/kv/kvserver/replica_raft_quiesce.go


-	if wakeLeader {
+	} else if st.RaftState == raft.StateFollower && wakeLeader {


Why not in StateCandidate? Waking the leader doesn't make sense because the candidate "doesn't know" the leader?

Correct, a candidate has no leader, and won't send proposals.

pav-kv · 2023-06-20T09:53:58Z

pkg/kv/kvserver/store_raft.go

@@ -705,7 +705,7 @@ func (s *Store) nodeIsLiveCallback(l livenesspb.Liveness) {
 		lagging := r.mu.laggingFollowersOnQuiesce
 		r.mu.RUnlock()
 		if quiescent && lagging.MemberStale(l) {
-			r.maybeUnquiesce(false /* wakeLeader */, true /* mayCampaign */)
+			r.maybeUnquiesce(false /* wakeLeader */, false /* mayCampaign */) // already leader


Why do we know we're already leader here?

Because laggingFollowersOnQuiesce is only non-nil on the leader. Conceptually, only the leader keeps track of lagging followers, because only leaders have followers.

Epic: none Release note: None

erikgrinaker · 2023-06-20T11:11:05Z

TFTRs!

bors r+

craig · 2023-06-20T12:05:42Z

Build succeeded:

Bazel Essential CI (Cockroach)

erikgrinaker self-assigned this Jun 16, 2023

erikgrinaker changed the title ~~Raft unquiesce cleanup~~ kvserver: clean up replica unquiescence Jun 16, 2023

erikgrinaker requested review from tbg and pav-kv June 16, 2023 12:40

erikgrinaker force-pushed the raft-unquiesce-cleanup branch from 3df693c to df4b6bc Compare June 16, 2023 13:17

erikgrinaker marked this pull request as ready for review June 16, 2023 13:18

erikgrinaker requested a review from a team June 16, 2023 13:18

erikgrinaker requested a review from a team as a code owner June 16, 2023 13:18

erikgrinaker force-pushed the raft-unquiesce-cleanup branch 2 times, most recently from 0ea89a1 to 6be9e48 Compare June 16, 2023 14:24

erikgrinaker force-pushed the raft-unquiesce-cleanup branch 3 times, most recently from c749770 to 7afc8a0 Compare June 17, 2023 14:43

erikgrinaker mentioned this pull request Jun 18, 2023

kvserver: don't campaign when unquiescing for a Raft message #105126

Merged

erikgrinaker force-pushed the raft-unquiesce-cleanup branch from 7afc8a0 to 24459ca Compare June 19, 2023 17:57

tbg approved these changes Jun 20, 2023

View reviewed changes

erikgrinaker added 7 commits June 20, 2023 09:36

kvserver: remove stale maybeQuiesce TODO

b798cc0

The leader won't quiesce if followers aren't caught up. Epic: none Release note: None

kvserver: omit unnecessary campaign checks on leader unquiescence

50d7264

This patch changes a couple of `maybeUnquiesce()` call sites to not attempt to campaign when we know the replica must already be the leader. Epic: none Release note: None

erikgrinaker force-pushed the raft-unquiesce-cleanup branch from 24459ca to 50d7264 Compare June 20, 2023 09:48

pav-kv approved these changes Jun 20, 2023

View reviewed changes

kvserver: use r-prefix for range IDs in quiescence logs

3dd1a37

Epic: none Release note: None

craig bot merged commit a416398 into cockroachdb:master Jun 20, 2023

erikgrinaker mentioned this pull request Jun 21, 2023

roachtest: admission-control/index-backfill failed #105260

Closed

erikgrinaker deleted the raft-unquiesce-cleanup branch November 14, 2023 10:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kvserver: clean up replica unquiescence #105041

kvserver: clean up replica unquiescence #105041

erikgrinaker commented Jun 16, 2023 •

edited

Loading

cockroach-teamcity commented Jun 16, 2023

erikgrinaker commented Jun 16, 2023 •

edited

Loading

erikgrinaker commented Jun 16, 2023

erikgrinaker commented Jun 16, 2023 •

edited

Loading

erikgrinaker commented Jun 16, 2023

tbg left a comment

pav-kv Jun 20, 2023

erikgrinaker Jun 20, 2023

erikgrinaker Jun 20, 2023

pav-kv Jun 20, 2023

erikgrinaker Jun 20, 2023 •

edited

Loading

pav-kv Jun 20, 2023

erikgrinaker Jun 20, 2023 •

edited

Loading

pav-kv Jun 20, 2023

erikgrinaker Jun 20, 2023 •

edited

Loading

erikgrinaker commented Jun 20, 2023

craig bot commented Jun 20, 2023

		@@ -495,15 +495,7 @@ func (r *Replica) GetQueueLastProcessed(ctx context.Context, queue string) (hlc.
		}

		func (r *Replica) MaybeUnquiesce() bool {


		if wakeLeader {
		} else if st.RaftState == raft.StateFollower && wakeLeader {

kvserver: clean up replica unquiescence #105041

kvserver: clean up replica unquiescence #105041

Conversation

erikgrinaker commented Jun 16, 2023 • edited Loading

cockroach-teamcity commented Jun 16, 2023

erikgrinaker commented Jun 16, 2023 • edited Loading

erikgrinaker commented Jun 16, 2023

erikgrinaker commented Jun 16, 2023 • edited Loading

erikgrinaker commented Jun 16, 2023

tbg left a comment

Choose a reason for hiding this comment

pav-kv Jun 20, 2023

Choose a reason for hiding this comment

erikgrinaker Jun 20, 2023

Choose a reason for hiding this comment

erikgrinaker Jun 20, 2023

Choose a reason for hiding this comment

pav-kv Jun 20, 2023

Choose a reason for hiding this comment

erikgrinaker Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

pav-kv Jun 20, 2023

Choose a reason for hiding this comment

erikgrinaker Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

pav-kv Jun 20, 2023

Choose a reason for hiding this comment

erikgrinaker Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

erikgrinaker commented Jun 20, 2023

craig bot commented Jun 20, 2023

erikgrinaker commented Jun 16, 2023 •

edited

Loading

erikgrinaker commented Jun 16, 2023 •

edited

Loading

erikgrinaker commented Jun 16, 2023 •

edited

Loading

erikgrinaker Jun 20, 2023 •

edited

Loading

erikgrinaker Jun 20, 2023 •

edited

Loading

erikgrinaker Jun 20, 2023 •

edited

Loading