Skip to content
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

[v23.2.x] log_eviction_stm: further cleanup #12489

Merged
merged 2 commits into from
Jul 27, 2023

Conversation

andrwng
Copy link
Contributor

@andrwng andrwng commented Jul 27, 2023

Backport of #12476

Removes the queue-based signaling in log_eviction_stm, which proved to
be a bit error prone.

Instead, this reverts to the old implementation using a
condition_variable, but leaves out the offset monitor, which doesn't
seem needed.

Now, either storage or delete-records will bump their respective
offsets, and the event-processing loop will take the higher and truncate
to it. If this isn't finished, the process is repeated after some
timeout.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.2.x
  • v23.1.x
  • v22.3.x

Release Notes

  • none

andrwng and others added 2 commits July 27, 2023 08:46
Removes the queue-based signaling in log_eviction_stm, which proved to
be a bit error prone.

Instead, this reverts to the old implementation using a
condition_variable, but leaves out the offset monitor, which doesn't
seem needed.

Now, either storage or delete-records will bump their respective
offsets, and the event-processing loop will take the higher and truncate
to it. If this isn't finished, the process is repeated after some
timeout.

(cherry picked from commit 6a723a7)
- The log_eviction_stm had an issue where a deadlock could occur if an
event from storage requested to evict at an offset that was below the
raft last snapshot index.

- This is because the code in the above scenario would wait but then go
down a path where .notify() was never called.

- This unit test reproduces the scenario described and ensures that
future edits to the eviction_stm do not re-introduce this problem.

(cherry picked from commit aa7b9fe)
@andrwng andrwng marked this pull request as ready for review July 27, 2023 16:10
@piyushredpanda piyushredpanda merged commit 10e4123 into redpanda-data:v23.2.x Jul 27, 2023
10 of 11 checks passed
@BenPope BenPope added the kind/backport PRs targeting a stable branch label Aug 7, 2023
@BenPope BenPope added this to the v23.2.3 milestone Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/redpanda kind/backport PRs targeting a stable branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants