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

Fix transaction visibility problem #3036

Merged
merged 3 commits into from Nov 20, 2021

Conversation

rystsov
Copy link
Contributor

@rystsov rystsov commented Nov 20, 2021

We control visibility of the transactions by using last_stable_offset() to set the maximum offset a reader may consume. See make_reader and log_reader_config::max_offset.

Because last_stable_offset() returns supremum of the consumable offsets and make_reader expects maximum it led to an off by one error and made records of the aborted transactions visible.

Fixing the issue by converting supremum to maximum.

How was the fix tested?

  • reproduced the problem locally
  • wrote a ducktape test, checked that it fails
  • applied the fix
  • checked that the test passes

Fixes https://github.com/vectorizedio/support/issues/176

We control visibility of the transactions by using last_stable_offset()
to set the maximum offset a reader may consume. See make_reader and
log_reader_config::max_offset.

Because last_stable_offset() returns supremum of the consumable offsets
and make_reader expects maximum it led to an off by one error and made
records of the aborted transactions visible.

Fixing the issue by converting supremum to maximum.

Fixes https://github.com/vectorizedio/support/issues/176
@rystsov rystsov requested review from dotnwat and twmb November 20, 2021 00:36
@rystsov rystsov changed the title Fix tx visibility problem Fix transaction visibility problem Nov 20, 2021
@rystsov rystsov added this to the v21.10.2 milestone Nov 20, 2021
Adding a ducktape test to prevent regression in the future
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.

TIL supremum

Fix looks good to me. Just a question about the future

src/v/kafka/server/handlers/fetch.cc Show resolved Hide resolved
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.

None yet

3 participants