Fixed checking if append_entries_request batches are already present in follower log
#25018
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-matching-entries-check
branch
from
d7a60fa to
153a9c2
Compare
bashtanov
reviewed
Feb 4, 2025
CI test resultstest results on build#61563
test results on build#61682
test results on build#61910
|
bashtanov
reviewed
Feb 5, 2025
src/v/raft/tests/raft_fixture.h
Outdated
| private: | ||
| model::node_id _id; | ||
| model::revision_id _revision; | ||
| prefix_logger _logger; | ||
| ss::sstring _base_directory; | ||
| config::mock_property<size_t> _max_inflight_requests{16}; | ||
| config::mock_property<size_t> _max_queued_bytes{1_MiB}; | ||
| config::mock_property<size_t> _default_recovery_read_size{32_KiB}; |
There was a problem hiding this comment.
any reason we change it for existing tests?
There was a problem hiding this comment.
no particular reason, i will make sure it is the same as before
bashtanov
reviewed
Feb 5, 2025
bashtanov
reviewed
Feb 5, 2025
Why is that? Anything wrong with the macro? AFAIK it's meant to work with both gtest and boost. |
mmaslankaprv
force-pushed
the
fix-matching-entries-check
branch
2 times, most recently
from
12b3024 to
5c4c17b
Compare
bashtanov
reviewed
Feb 6, 2025
mmaslankaprv
force-pushed
the
fix-matching-entries-check
branch
from
5c4c17b to
ed45488
Compare
bashtanov
reviewed
Feb 7, 2025
src/v/raft/tests/raft_fixture.cc
Outdated
Comment on lines
707
to
711
| std::ranges::copy( | ||
| _nodes | std::views::keys | ||
| | std::views::filter( | ||
| [leader_id](model::node_id id) { return id != leader_id; }), | ||
| std::back_inserter(followers)); |
| /** | ||
| * Recover communication and wait for the intercept to trigger | ||
| */ | ||
| new_leader_node.reset_dispatch_handlers(); |
There was a problem hiding this comment.
This will enable the new leader to send vote requests to the old leader. I guess it won't anyway, as it has been elected already. Do we need this?
| * Recover communication and wait for the intercept to trigger | ||
| */ | ||
| new_leader_node.reset_dispatch_handlers(); | ||
| co_await reply_intercepted.wait([&] { return intercept_count > 5; }); |
There was a problem hiding this comment.
We don't produce anything after the second election. What are the 20+ messages that are replicated from the new leader to the old one?
There was a problem hiding this comment.
there will be no new messages replicated, only recovery append entry requests finally leading to truncation
When follower receives an append entries request that `prev_log_index` is smaller than its own `prev_log_index` it validates if the batches from the request matches (by checking a batch offset and corresponding term) its own log. It that is the case the batches are skipped to prevent truncation of valid batches and avoid the data loss. The validation of already matching batches was broken if they happened to be at the beginning of the log. In this case the `prev_log_index` is not initialized being negative. This case was not correctly handled by the logic calculating the next offset when checking matching batches. That lead to a situation in which a range of batches starting with 0 was never matching. Fixed the issue by correctly adjusting the `prev_log_index` if it is uninitialized. Signed-off-by: Michał Maślanka <michal@redpanda.com>
When follower receives an append entries request with the vector of records that are all present in its own log and their offsets and terms match it should reply with success and correct `last_dirty_log_index`. This way a leader instead of moving the follower `next_offset` backward can start recovery process and deliver batches which the follower is missing. Signed-off-by: Michał Maślanka <michal@redpanda.com>
Signed-off-by: Michał Maślanka <michal@redpanda.com>
Assertion triggered in function body are not propagated to the test itself. Change the method to throw an exception in case of timeout instead of using an assertion. Signed-off-by: Michał Maślanka <michal@redpanda.com>
The reply interceptor allows test creator to modify or drop the reply that is about to be processed by the RPC requester. This allow tests to take more control over the Raft protocol behavior and test some rare edge cases which might be hard to trigger otherwise. Signed-off-by: Michał Maślanka <michal@redpanda.com>
Signed-off-by: Michał Maślanka <michal@redpanda.com>
mmaslankaprv
force-pushed
the
fix-matching-entries-check
branch
from
ed45488 to
59e43a7
Compare
ztlpn
approved these changes
Feb 18, 2025
|
/backport v24.3.x |
|
/backport v24.2.x |
|
/backport v24.1.x |
|
Failed to create a backport PR to v24.3.x branch. I tried: |
|
Failed to create a backport PR to v24.2.x branch. I tried: |
|
Failed to create a backport PR to v24.1.x branch. I tried: |
This was referenced Mar 12, 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.
Background
When follower receives an append entries request that
prev_log_indexis smaller than its ownprev_log_indexit validates if the batches from the request matches (by checking a batch offset and corresponding term) its own log. It that is the case the batches are skipped to prevent truncation of valid batches and avoid the data loss.Negative
append_entries_request::prev_log_indexThe validation of already matching batches was broken if they happened to be at the beginning of the log. In this case the
prev_log_indexis not initialised. This case was not correctly handled by the logic calculating the next offset when checking matching batches.Replying with success when all request batches match
When follower receives an append entries request with the vector of records that are all present in its own log and their offsets and terms match it should reply with success and correct
last_dirty_log_index.This way a leader instead of moving the follower
next_offsetbackward can start recovery process and deliver batches which the follower is missing.Backports Required
Release Notes
Bug Fixes