Fix stepping down on timeout #24590
Merged
Fix stepping down on timeout #24590
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mmaslankaprv
force-pushed
the
fix-stepping-down-on-timeout
branch
3 times, most recently
from
717a5fa to
c321e29
Compare
mmaslankaprv
requested review from
dotnwat,
bharathv,
bashtanov,
ztlpn and
travisdowns
Retry command for Build#59862please wait until all jobs are finished before running the slash command |
CI test resultstest results on build#59862test results on build#59902
test results on build#59984
|
mmaslankaprv
force-pushed
the
fix-stepping-down-on-timeout
branch
from
c321e29 to
e203f89
Compare
Retry command for Build#59902please wait until all jobs are finished before running the slash command |
mmaslankaprv
force-pushed
the
fix-stepping-down-on-timeout
branch
from
e203f89 to
592fc64
Compare
Retry command for Build#59984please wait until all jobs are finished before running the slash command |
bashtanov
reviewed
Dec 20, 2024
| @@ -489,6 +490,8 @@ ss::future<> raft_node_instance::stop() { | |||
| vlog(_logger.debug, "stopping protocol"); | |||
| co_await _buffered_protocol->stop(); | |||
| co_await _protocol->stop(); | |||
| // release f_log pointer before stopping raft | |||
There was a problem hiding this comment.
Any reason we do it before? It should not matter, as it's a shared pointer, so the underlying object will only die when _raft is gone.
Same question for stopping log before deleting raft, but it's out of scope of this PR.
The `raft::reply_result::follower_busy` is indicating that the follower was unable to process the heartbeat fast enough to generate a response. Renaming the reply from `timeout` will make it less confusing for the reader and differentiate the error code from an RPC timeout. Signed-off-by: Michał Maślanka <[email protected]>
Signed-off-by: Michał Maślanka <[email protected]>
Wired raft RPC service handler into Raft fixture to make the tests more accurate and cover the service code with tests. Signed-off-by: Michał Maślanka <[email protected]>
Propagating timeout to the node sending RPC request is crucial for accurate testing of Raft implementation. Signed-off-by: Michał Maślanka <[email protected]>
Added a wrapper around the `storage::log` allowing us to inject storage layer failures in Raft fixture tests. Signed-off-by: Michał Maślanka <[email protected]>
When follower is busy it may fail fast processing full heartbeat requests sent by the leader. In this case a follower RPC handler sets the `follower_busy` result in heartbeat_reply. Leader should still treat a follower replica as online in this case. The replica hosting node must be online to reply with the `follower_busy` error. This way we prevent to eager leader step downs when follower replicas are slow. Signed-off-by: Michał Maślanka <[email protected]>
Signed-off-by: Michał Maślanka <[email protected]>
mmaslankaprv
force-pushed
the
fix-stepping-down-on-timeout
branch
from
592fc64 to
67e7c6e
Compare
Right, and I just to clarify, it is not exactly that the leader should treat the follower as "online" for the purposes of how it interacts with the follower (e.g., decisions about whether to send it rpc payloads), but that it should not consider itself isolated from that follower when making step down decisions, right? |
travisdowns
reviewed
Dec 23, 2024
| @@ -32,7 +32,7 @@ enum class reply_result : uint8_t { | |||
| success, | |||
| failure, | |||
| group_unavailable, | |||
| timeout | |||
| follower_busy | |||
There was a problem hiding this comment.
I a bit confused, is the timeout case no longer possible?
Don't we have two cases at least: follow replies immediately with "busy", and also follower never replies?
There was a problem hiding this comment.
I see, I guess the timeout case never ends using the reply_result, it will be handled in a different path: these codes are only for cases where an RPC was actually received, right?
travisdowns
approved these changes
Jan 2, 2025
|
/backport v24.3.x |
|
/backport v24.2.x |
|
Failed to create a backport PR to v24.2.x branch. I tried: |
|
Failed to create a backport PR to v24.3.x branch. I tried: |
This was referenced Jan 2, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When follower is busy it may fail fast processing full heartbeat
requests sent by the leader. In this case a follower RPC handler sets
the
follower_busyresult in heartbeat_reply. Leader should still treata follower replica as online in this case. The replica hosting node must
be online to reply with the
follower_busyerror.This way we prevent to eager leader step downs when follower replicas
are slow.
Backports Required
Release Notes
Improvements