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

Transactions: remove expired abort indexes #5556

Merged
merged 3 commits into from
Aug 19, 2022

Conversation

rystsov
Copy link
Contributor

@rystsov rystsov commented Jul 21, 2022

Cover letter

Once a log was evicted up to an offset X it's wasteful to store abort indexes covering the offsets before X (the abort index is used only for the existing offsets). The commit drops the expired abort indexes during the snapshot procedure.

Release notes

  • none

Copy link
Contributor

@ajfabbri ajfabbri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. I am asking for some python type annotations then this should be ready to go.

tests/rptest/tests/tx_abort_index_test.py Show resolved Hide resolved
src/v/cluster/rm_stm.cc Show resolved Hide resolved
src/v/cluster/rm_stm.cc Outdated Show resolved Hide resolved
src/v/cluster/rm_stm.cc Outdated Show resolved Hide resolved
src/v/cluster/rm_stm.cc Show resolved Hide resolved
src/v/cluster/rm_stm.cc Outdated Show resolved Hide resolved
tests/rptest/tests/tx_abort_index_test.py Outdated Show resolved Hide resolved
@mmedenjak mmedenjak added kind/enhance New feature or request area/transactions and removed area/redpanda labels Jul 22, 2022
src/v/cluster/rm_stm.cc Show resolved Hide resolved
src/v/cluster/rm_stm.cc Outdated Show resolved Hide resolved
src/v/cluster/rm_stm.cc Show resolved Hide resolved
Once a log was evicted up to an offset X it's wasteful to store abort
indexes covering the offsets before X (the abort index is used only
for the existing offsets). The commit drops the expired abort indexes
during the snapshot procedure.
@rystsov
Copy link
Contributor Author

rystsov commented Aug 6, 2022

Build failure: #5868

Copy link
Member

@dotnwat dotnwat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rystsov PR looks good to me!

but I think there is a potential race condition that could lead to a segfault. where the race is possible or is protected against some how is not clear. could you take a look? i left a comment with the concern.

src/v/cluster/rm_stm.cc Show resolved Hide resolved
src/v/cluster/rm_stm.cc Show resolved Hide resolved
Copy link
Member

@dotnwat dotnwat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a linter error

@rystsov
Copy link
Contributor Author

rystsov commented Aug 13, 2022

there is a linter error

not anymore

Copy link
Contributor

@bharathv bharathv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patch looks ok to me if we decided not to make any changes to in-memory representation... one last question about implications to shadow indexing relying on this data.

src/v/cluster/rm_stm.cc Show resolved Hide resolved
src/v/cluster/rm_stm.cc Show resolved Hide resolved
Copy link
Member

@dotnwat dotnwat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good. just a couple questions

src/v/cluster/rm_stm.cc Show resolved Hide resolved
src/v/cluster/rm_stm.cc Show resolved Hide resolved
@rystsov rystsov merged commit 2fbdd52 into redpanda-data:dev Aug 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants