-
Notifications
You must be signed in to change notification settings - Fork 580
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.
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
stm_manager: tighten interaction between bg application and snapshots #18576
Conversation
840342c
to
436d3eb
Compare
umm, looks like the assert is still firing, I'll have to take another look. |
wait for background applicators to finish before applying raft snapshots, else there is a danger of next_to_apply offset moving backwards with interleaved bg_apply and snapshot fibers.
/ci-repeat 1 |
skipped ducktape retry in https://buildkite.com/redpanda/redpanda/builds/51118#01908002-10a7-4a12-b098-2a98ca78ea56: |
/backport v24.1.x |
/backport v23.3.x |
@@ -276,6 +281,7 @@ ss::future<> state_machine_manager::do_apply_raft_snapshot( | |||
}); | |||
} | |||
_next = model::next_offset(metadata.last_included_index); | |||
background_apply_units.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the reason for clearing the units here compared to making acquire_background_apply_mutexes
be RAII (e.g. with_background_apply_mutexes(...
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to make it explicit to the reader..
I wanted to do this.
ss::future<> state_machine_manager::do_apply_raft_snapshot(
snapshot_metadata metadata,
storage::snapshot_reader& reader,
std::vector<ssx::semaphore_units> background_apply_units) {
over
ss::future<> state_machine_manager::do_apply_raft_snapshot(
snapshot_metadata metadata,
storage::snapshot_reader& reader,
std::vector<ssx::semaphore_units>) {
that signifies these units are from bg_applicators but the former won't compile because background_apply_units
remains unused, so I had to use it some how, so explicitly cleared the vector.
I guess I could've used [[maybe_unused]]
@bharathv I think the release note could add a little context: https://github.com/redpanda-data/redpanda/releases/tag/untagged-e8ce2cd9a5a987a31e3f |
Done. |
wait for background applicators to finish before applying raft
snapshots, else there is a danger of next_to_apply offset moving
backwards with interleaved bg_apply and snapshot fibers.
Fixes #18415
Backports Required
Release Notes
Bug Fixes