You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by "ivandika3 (via GitHub)" <gi...@apache.org> on 2026/01/14 09:42:57 UTC
[PR] RATIS-2382. Support skip leadership check during ReadIndex [ratis]
ivandika3 opened a new pull request, #1334:
URL: https://github.com/apache/ratis/pull/1334
## What changes were proposed in this pull request?
Please refer to https://issues.apache.org/jira/browse/RATIS-2382
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-2382
## How was this patch tested?
CI (https://github.com/ivandika3/ratis/actions/runs/20984539273)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2382. Support skip leadership check during ReadIndex [ratis]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #1334:
URL: https://github.com/apache/ratis/pull/1334#discussion_r2696608101
##########
ratis-docs/src/site/markdown/configurations.md:
##########
@@ -227,6 +227,29 @@ if it fails to receive any RPC responses from this peer within this specified ti
| **Default** | false |
+| **Property** | `raft.server.read.read-index.heartbeat.skip.enabled` |
Review Comment:
@OneSizeFitsQuorum Thanks for the review. I was also grappling with whether "leadership check" is a better name. However, my reasoning of choosing "heartbeat" is because we already have a `LeaderStateImpl#checkLeadership` to decide whether to step down. `LeaderStateImpl#checkLeadership` does not send heartbeat, it simply checks the last RPC response time of the majority. Therefore, I picked "heartbeat" to explicitly mention the RPC that is going to be skipped.
That said, I'm ok if the community decide "leadership-check" is a more fitting name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2382. Support skip leadership check during ReadIndex [ratis]
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum commented on code in PR #1334:
URL: https://github.com/apache/ratis/pull/1334#discussion_r2696566259
##########
ratis-docs/src/site/markdown/configurations.md:
##########
@@ -227,6 +227,29 @@ if it fails to receive any RPC responses from this peer within this specified ti
| **Default** | false |
+| **Property** | `raft.server.read.read-index.heartbeat.skip.enabled` |
Review Comment:
LGTM for the idea and code.
Just want to discuss whether heartbeat.skip.enabled is a good name, I think maybe `raft.server.read.read-index.leadership-check.skip.enabled`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2382. Support skip leadership check during ReadIndex [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #1334:
URL: https://github.com/apache/ratis/pull/1334#issuecomment-3869978922
@ivandika3 , thanks for working on this!
@OneSizeFitsQuorum , thanks for reviewing this!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2382. Support skip leadership check during ReadIndex [ratis]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #1334:
URL: https://github.com/apache/ratis/pull/1334#discussion_r2780609919
##########
ratis-docs/src/site/markdown/configurations.md:
##########
@@ -227,6 +227,29 @@ if it fails to receive any RPC responses from this peer within this specified ti
| **Default** | false |
+| **Property** | `raft.server.read.read-index.leadership-heartbeat-check.skip.enabled` |
+|:----------------|:----------------------------------------------------------------------|
+| **Description** | whether to skip the leadership check heartbeat for read index. |
+| **Type** | boolean |
+| **Default** | false |
+
+Note that although enabling `leadership-heartbeat-check.skip.enabled` reduce the RTT due to the leadership check heartbeat,
+it causes reads to not be linearizable in some cases.
+This is because without the leadership check heartbeats, the leader might not be the latest leader
+and might serve stale reads.
+There might be a small period of time when there is a split brain and two Raft peers that believe that
+they are leaders (old leader and new leader where old leader's term < new leader's term).
+The old leader might not have detected that it has lost majority heartbeats and stepped down or the old leader
+has not received any RPC with higher term which forces the old leader to steps down.
+Without the leadership check heartbeat which should detect that the old leader is no longer the latest leader and
+force the old leader to step down, the old leader ReadIndex might return an index that is lower than
+the new leader's applied index.
+This means that the old leader might return stale data which is not linearizable by definition.
+However, this situation should happen only abnormal situations when the Raft group encounters a leader election
+due to network partition between the old leader and the other majority quorum (which will elect a new leader).
+Therefore, this might be an acceptable tradeoff for applications that seek to improve the linearizable read
+performance.
Review Comment:
Thanks, updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2382. Support skip leadership check during ReadIndex [ratis]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #1334:
URL: https://github.com/apache/ratis/pull/1334#discussion_r2780612467
##########
ratis-docs/src/site/markdown/configurations.md:
##########
@@ -227,6 +227,29 @@ if it fails to receive any RPC responses from this peer within this specified ti
| **Default** | false |
+| **Property** | `raft.server.read.read-index.heartbeat.skip.enabled` |
Review Comment:
Good point to make it closer to lease since disabling heartbeat check essentially removed some of the leader lease guarantee.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2382. Support skip leadership check during ReadIndex [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #1334:
URL: https://github.com/apache/ratis/pull/1334
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2382. Support skip leadership check during ReadIndex [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1334:
URL: https://github.com/apache/ratis/pull/1334#discussion_r2780495435
##########
ratis-docs/src/site/markdown/configurations.md:
##########
@@ -227,6 +227,29 @@ if it fails to receive any RPC responses from this peer within this specified ti
| **Default** | false |
+| **Property** | `raft.server.read.read-index.heartbeat.skip.enabled` |
Review Comment:
> What about raft.server.read.read-index.leadership-heartbeat-check.skip.enabled?
Sorry for coming late:
- Since this conf is closely related to `raft.server.read.leader.lease.enabled`, they should be next to each other.
- The default "check.skip.enabled == false" is a double negative.
So, how about `raft.server.read.leader.heartbeat-check.enabled` (default is true).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2382. Support skip leadership check during ReadIndex [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1334:
URL: https://github.com/apache/ratis/pull/1334#discussion_r2780555566
##########
ratis-docs/src/site/markdown/configurations.md:
##########
@@ -227,6 +227,29 @@ if it fails to receive any RPC responses from this peer within this specified ti
| **Default** | false |
+| **Property** | `raft.server.read.read-index.leadership-heartbeat-check.skip.enabled` |
+|:----------------|:----------------------------------------------------------------------|
+| **Description** | whether to skip the leadership check heartbeat for read index. |
+| **Type** | boolean |
+| **Default** | false |
+
+Note that although enabling `leadership-heartbeat-check.skip.enabled` reduce the RTT due to the leadership check heartbeat,
+it causes reads to not be linearizable in some cases.
+This is because without the leadership check heartbeats, the leader might not be the latest leader
+and might serve stale reads.
+There might be a small period of time when there is a split brain and two Raft peers that believe that
+they are leaders (old leader and new leader where old leader's term < new leader's term).
+The old leader might not have detected that it has lost majority heartbeats and stepped down or the old leader
+has not received any RPC with higher term which forces the old leader to steps down.
+Without the leadership check heartbeat which should detect that the old leader is no longer the latest leader and
+force the old leader to step down, the old leader ReadIndex might return an index that is lower than
+the new leader's applied index.
+This means that the old leader might return stale data which is not linearizable by definition.
+However, this situation should happen only abnormal situations when the Raft group encounters a leader election
+due to network partition between the old leader and the other majority quorum (which will elect a new leader).
+Therefore, this might be an acceptable tradeoff for applications that seek to improve the linearizable read
+performance.
Review Comment:
Thanks for adding the doc. Below is my suggestion:
```md
| **Property** | `raft.server.read.leader.heartbeat-check.enabled` |
|:----------------|:--------------------------------------------------|
| **Description** | whether to check heartbeat for read index. |
| **Type** | boolean |
| **Default** | true |
Note that the original read index algorithm requires heartbeat check
in order to guarantee linearizable read.
By setting this property to false,
it reduces the RTT by eliminating the heartbeat check.
However, it might cause the reads not to be linearizable in a split-brain case.
Without the heartbeat check, a leader might not be the latest leader
and, as a result, it might serve stale reads.
When there is a split brain, there might be a small period of time
that the (old) leader has lost majority heartbeats but have not yet detected it.
As the same time, a new leader is elected by a majority of peers.
Then, the old leader might serve stale data
since it does not have the transactions committed by the new leaders.
Since such split-brain case is supposed to be rare,
it might be an acceptable tradeoff for applications that
seek to improve the linearizable read performance.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2382. Support skip leadership check during ReadIndex [ratis]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #1334:
URL: https://github.com/apache/ratis/pull/1334#discussion_r2780408854
##########
ratis-docs/src/site/markdown/configurations.md:
##########
@@ -227,6 +227,29 @@ if it fails to receive any RPC responses from this peer within this specified ti
| **Default** | false |
+| **Property** | `raft.server.read.read-index.heartbeat.skip.enabled` |
Review Comment:
@OneSizeFitsQuorum Sounds good, updated. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2382. Support skip leadership check during ReadIndex [ratis]
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum commented on code in PR #1334:
URL: https://github.com/apache/ratis/pull/1334#discussion_r2696566259
##########
ratis-docs/src/site/markdown/configurations.md:
##########
@@ -227,6 +227,29 @@ if it fails to receive any RPC responses from this peer within this specified ti
| **Default** | false |
+| **Property** | `raft.server.read.read-index.heartbeat.skip.enabled` |
Review Comment:
LGTM for the code.
Just want to discuss whether heartbeat.skip.enabled is a good name, I think maybe `raft.server.read.read-index.leadership-check.skip.enabled`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2382. Support skip leadership check during ReadIndex [ratis]
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum commented on code in PR #1334:
URL: https://github.com/apache/ratis/pull/1334#discussion_r2778735766
##########
ratis-docs/src/site/markdown/configurations.md:
##########
@@ -227,6 +227,29 @@ if it fails to receive any RPC responses from this peer within this specified ti
| **Default** | false |
+| **Property** | `raft.server.read.read-index.heartbeat.skip.enabled` |
Review Comment:
What about `raft.server.read.read-index.leadership-heartbeat-check.skip.enabled`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2382. Support skip leadership check during ReadIndex [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1334:
URL: https://github.com/apache/ratis/pull/1334#discussion_r2780495435
##########
ratis-docs/src/site/markdown/configurations.md:
##########
@@ -227,6 +227,29 @@ if it fails to receive any RPC responses from this peer within this specified ti
| **Default** | false |
+| **Property** | `raft.server.read.read-index.heartbeat.skip.enabled` |
Review Comment:
> What about raft.server.read.read-index.leadership-heartbeat-check.skip.enabled?
Sorry for coming late:
- Since this conf is closely related to `raft.server.read.leader.lease.enabled`, they should be next to each other.
- The default "skip.enabled == false" is a double negative.
So, how about `raft.server.read.leader.heartbeat-check.enabled` (default is true).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2382. Support skip leadership check during ReadIndex [ratis]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on PR #1334:
URL: https://github.com/apache/ratis/pull/1334#issuecomment-3870121973
Thanks @OneSizeFitsQuorum @szetszwo for the reviews.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2382. Support skip leadership check during ReadIndex [ratis]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #1334:
URL: https://github.com/apache/ratis/pull/1334#discussion_r2696608101
##########
ratis-docs/src/site/markdown/configurations.md:
##########
@@ -227,6 +227,29 @@ if it fails to receive any RPC responses from this peer within this specified ti
| **Default** | false |
+| **Property** | `raft.server.read.read-index.heartbeat.skip.enabled` |
Review Comment:
@OneSizeFitsQuorum Thanks for the review. I was also grappling with whether "leadership check" is a better name. However, my reasoning of choosing "heartbeat" is because we already have a `LeaderStateImpl#checkLeadership` to decide whether to leader need to step down due to lost majority heartbeats. `LeaderStateImpl#checkLeadership` does not send heartbeat, it simply checks the last RPC response time of the majority. Therefore, I picked "heartbeat" to explicitly mention the RPC that is going to be skipped.
That said, I'm ok if the community decide "leadership-check" is a more fitting name.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org