Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

raft: support disabling heartbeats #125248

Closed
nvanbenschoten opened this issue Jun 6, 2024 · 3 comments
Closed

raft: support disabling heartbeats #125248

nvanbenschoten opened this issue Jun 6, 2024 · 3 comments
Labels
A-kv-replication Relating to Raft, consensus, and coordination. A-leader-leases Related to the introduction of leader leases C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. T-kv KV Team

Comments

@nvanbenschoten
Copy link
Member

nvanbenschoten commented Jun 6, 2024 •
edited by exalate-issue-sync bot
Loading

Raft fortification over Store Liveness present the opportunity to remove raft heartbeats. We should do so once these concepts have been migrated in and once the role of raft heartbeats to drive commit index propagation has been removed (#122100).

Jira issue: CRDB-39337

Epic CRDB-37522
@nvanbenschoten nvanbenschoten added C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. A-kv-replication Relating to Raft, consensus, and coordination. T-kv KV Team A-leader-leases Related to the introduction of leader leases labels Jun 6, 2024
@blathers-crl blathers[crl]
Copy link

blathers-crl bot commented Jun 6, 2024

cc @cockroachdb/replication

@nvanbenschoten nvanbenschoten mentioned this issue Jun 6, 2024
Open
@pav-kv
Copy link
Collaborator

pav-kv commented Jun 6, 2024 •
edited
Loading

#122100 is not so much about removing the dependency on heartbeats to drive commit index - the issue makes the latter more efficient.

Today, we have 2 mechanisms: Commit is delivered in MsgApps and heartbeats. Heartbeats mostly close the liveness gap, in cases when MsgApps are dropped. We should probably have another issue (upd: filed #125266) to make MsgApp commit index propagation reliable, so that we don't need heartbeats to close the gap.

@nvanbenschoten nvanbenschoten mentioned this issue Aug 26, 2024
Merged
craig bot pushed a commit that referenced this issue Sep 26, 2024
@nvanbenschoten @sumeerbhola
Merge #129692 #131439
c56def7 
129692: kv: don't quiesce leader leases r=nvanbenschoten a=nvanbenschoten

Closes #129688.

A fortified raft leader will not send raft heartbeats after we address #125248, so quiescence will not be needed. All liveness decisions are based on store liveness communication, which is cheap enough to not need a notion of quiescence.

We should not merge this until after we address #125248.

Release note: None

131439: rac2,replica_rac2: order replicaSendStream.mu before Replica.mu r=kvoli a=sumeerbhola

The other way around is too hard to work with when we add pull mode. We will call MakeMsgAppAndAssumeSent from inside a replicaSendStream and will be adjusting replicaSendStream state before and after that call. During that call Replica.mu needs to be held, since Raft state for this follower (like Next) is being modified. It is not convenient to release and reacquire replicaSendStream.mu just for this purpose.

Informs #130433

Epic: CRDB-37515

Release note: None

Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: sumeerbhola <[email protected]>
@nvanbenschoten
Copy link
Member Author

Addressed by #128497 and #131976.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-kv-replication Relating to Raft, consensus, and coordination. A-leader-leases Related to the introduction of leader leases C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. T-kv KV Team
Projects
No open projects
KV
Status: Incoming
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jlinder @pav-kv @nvanbenschoten