Skip to content

kvserver: add Replica.isRaftLeader fast path for ticks#94595

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
erikgrinaker:replica-is-raft-leader
Jan 3, 2023
Merged

kvserver: add Replica.isRaftLeader fast path for ticks#94595
craig[bot] merged 1 commit intocockroachdb:masterfrom
erikgrinaker:replica-is-raft-leader

Conversation

@erikgrinaker
Copy link
Copy Markdown
Contributor

@erikgrinaker erikgrinaker commented Jan 2, 2023 •
edited
Loading

This patch adds Replica.isRaftLeader() which can very cheaply determine whether the replica believes itself to be the current Raft leader. This reduces the Raft tick cost by 30% by nearly eliminating the maybeTransferRaftLeadershipToLeaseholderLocked CPU time.

Touches #94592.
Touches #94609.

Epic: none
Release note: None

@erikgrinaker erikgrinaker requested a review from tbg January 2, 2023 22:13
@erikgrinaker erikgrinaker self-assigned this Jan 2, 2023
@erikgrinaker erikgrinaker requested a review from a team January 2, 2023 22:13
@erikgrinaker erikgrinaker requested a review from a team as a code owner January 2, 2023 22:13
@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Jan 2, 2023

This change is Reviewable

@erikgrinaker erikgrinaker mentioned this pull request Jan 2, 2023
Merged
@erikgrinaker erikgrinaker force-pushed the replica-is-raft-leader branch from ab5a854 to 0a96cfb Compare January 2, 2023 23:03
@erikgrinaker erikgrinaker force-pushed the replica-is-raft-leader branch from 0a96cfb to 50c94ee Compare January 3, 2023 10:10
@erikgrinaker
Copy link
Copy Markdown
Contributor Author

erikgrinaker commented Jan 3, 2023

isRaftLeaderRLocked() can race with the actual Raft status, because we update leaderID in a separate critical section from the Raft ready processing. Added comments saying as much. I don't believe this is an issue for our purposes here, because we always double-check against the actual Raft status later, so in the worst case we have a false negative during ticks.

cockroach/pkg/kv/kvserver/replica_raft.go

Lines 946 to 958 in 3ee0a7a

r.mu.Lock()
// TODO(pavelkalinnikov): put logstore.RaftState to r.mu directly.
r.mu.lastIndex = state.LastIndex
r.mu.lastTerm = state.LastTerm
r.mu.raftLogSize = state.ByteSize
var becameLeader bool
if r.mu.leaderID != leaderID {
r.mu.leaderID = leaderID
// Clear the remote proposal set. Would have been nil already if not
// previously the leader.
becameLeader = r.mu.leaderID == r.replicaID
}
r.mu.Unlock()

@erikgrinaker
Copy link
Copy Markdown
Contributor Author

erikgrinaker commented Jan 3, 2023

bors r+

@erikgrinaker
kvserver: add Replica.isRaftLeader fast path for ticks
d400951 
This patch adds `Replica.isRaftLeader()` which can very cheaply
determine whether the replica believes itself to be the current Raft
leader. This reduces the Raft tick cost by 30% by nearly eliminating the
`maybeTransferRaftLeadershipToLeaseholderLocked` CPU time.

Epic: none
Release note: None
@erikgrinaker erikgrinaker force-pushed the replica-is-raft-leader branch from 50c94ee to d400951 Compare January 3, 2023 11:31
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jan 3, 2023

Canceled.

@erikgrinaker
Copy link
Copy Markdown
Contributor Author

erikgrinaker commented Jan 3, 2023

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jan 3, 2023

Build succeeded:

@craig craig bot merged commit 66419e9 into cockroachdb:master Jan 3, 2023
@erikgrinaker erikgrinaker deleted the replica-is-raft-leader branch January 3, 2023 13:01
@erikgrinaker erikgrinaker mentioned this pull request Jan 5, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tbg tbg tbg approved these changes

Assignees

@erikgrinaker erikgrinaker

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@erikgrinaker @cockroach-teamcity @tbg