Always use the latest configuration for followers metadata #19964
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
Removed comment mentioning that we should add applying snapshot to stm as this is already being done Signed-off-by: Michał Maślanka <michal@redpanda.com>
When install snapshot request in processed by the follower it may not always replace the follower log content. If the snapshot last included offset is smaller than the follower dirty offset the follower should prefix truncate all the data up to the snapshot last included offset but keep all the entries which offset is greater than snapshot last included offset. Fixed the update of follower metadata state as it was always using the snapshot configuration instead the latest from the configuration manager. Fixes: #core-internal/issues/1310 Signed-off-by: Michał Maślanka <michal@redpanda.com>
When install snapshot request is received by the current leader it must unconditionally step down. This is the same behavior as in the case of receiving an append entries request. Signed-off-by: Michał Maślanka <michal@redpanda.com>
mmaslankaprv
force-pushed
the
fix-assert-snapshot
branch
from
ad11e77 to
4d47efa
Compare
When install snapshot request is delayed and deliver to the node after it already made progress it can not lead to state inconsistencies. Added test validating handing of delayed `install_snapshot` requests. The test is validating behavior of both follower and the leader. Signed-off-by: Michał Maślanka <michal@redpanda.com>
bharathv
reviewed
Jun 24, 2024
| @@ -2247,6 +2247,7 @@ ss::future<> consensus::hydrate_snapshot() { | |||
| co_await truncate_to_latest_snapshot(truncate_cfg.value()); | |||
| } | |||
| _snapshot_size = co_await _snapshot_mgr.get_snapshot_size(); | |||
| update_follower_stats(_configuration_manager.get_latest()); | |||
There was a problem hiding this comment.
I was wondering if we should even hydrate a snapshot if the included_index < start offset, just return success and make it idempotent?
There was a problem hiding this comment.
I looked at this again, I think the fix makes sense but I think the issue though is L2241 already updates follower stats from snapshot config, so we are effectively doing it twice? Perhaps we should just swap L2242 & L2241 and make update_offset_from_snapshot() use latest config?
There was a problem hiding this comment.
i've removed the stats update from update_offset_from_snapshot. I am not sure if the stats should be updated from within that method as its name suggest updating offsets.
mmaslankaprv
force-pushed
the
fix-assert-snapshot
branch
from
4d47efa to
e73aa28
Compare
bharathv
approved these changes
Jun 25, 2024
|
/ci-repeat 1 |
|
/backport v24.1.x |
|
/backport v23.3.x |
|
Failed to create a backport PR to v23.3.x branch. I tried: |
This was referenced Jun 26, 2024
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 install snapshot request in processed by the follower it may not
always replace the follower log content. If the snapshot last included
offset is smaller than the follower dirty offset the follower should
prefix truncate all the data up to the snapshot last included offset but
keep all the entries which offset is greater than snapshot last included
offset.
Fixed the update of follower metadata state as it was always using the
snapshot configuration instead the latest from the configuration
manager.
Fixes: https://github.com/redpanda-data/core-internal/issues/1310
Backports Required
Release Notes
Bug Fixes