Fixed assertion triggered when fetching from empty topics with transactions enabled #3159
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
emaxerrno
reviewed
Dec 4, 2021
| @@ -134,7 +134,9 @@ static ss::future<read_result> read_from_partition( | |||
| auto lso = part.last_stable_offset(); | |||
| auto start_o = part.start_offset(); | |||
| // if we have no data read, return fast | |||
| if (hw < config.start_offset || config.skip_read) { | |||
| if ( | |||
| hw < config.start_offset || config.skip_read | |||
There was a problem hiding this comment.
added ducktape test, took me some time to reproduce this
It may happen that last stable offset is equal to log start offset. In this case when consumer requests a read with a `read_committed` isolation level it should receive no batches since the log is empty. Otherwise when records are present in the log we need to read up to LSO-1 since batch pointed by LSO is the first offset for which transactions may not be resolved. Added explicit check to partition read code path to return fast when requested max offset is smaller than start offset indicating no data should be returned. Signed-off-by: Michal Maslanka <michal@vectorized.io>
…verity When using trace log severity we should log more info to facilitate Fetch request debugging. The same level of logging is used in produce request, fetch handler is now consistent. Signed-off-by: Michal Maslanka <michal@vectorized.io>
Signed-off-by: Michal Maslanka <michal@vectorized.io>
Signed-off-by: Michal Maslanka <michal@vectorized.io>
Signed-off-by: Michal Maslanka <michal@vectorized.io>
mmaslankaprv
force-pushed
the
fix-assertion-fetch
branch
from
21b286d
to
45ae3cb
Compare
twmb
reviewed
Dec 6, 2021
|
|
||
| def consume(n=1): | ||
|
|
||
| out = rpk.consume(self.topic, group=consumer_group, n=n) |
There was a problem hiding this comment.
rpk now supports -f '%o'\n if you want to have offsets printed with newlines.
dotnwat
approved these changes
Dec 6, 2021
There was a problem hiding this comment.
lgtm.
Can you add an explanation of how this is related to transactions? transactions is in the PR title, but there doesn't seem to be any connection with transactions in the code nor the commit messages.
EDIT: just realized I missed since batch pointed by LSO is the first offset for which transactions may not be resolved.
All good!
This was referenced Dec 6, 2021
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Cover letter
It may happen that last stable offset is equal to log start offset.
In this case when consumer requests a read with a
read_committedisolation level it should receive no batches since the log is empty.
Otherwise when records are present in the log we need to read up to
LSO-1since batch pointed byLSOis the first offset for whichtransactions may not be resolved.
Added explicit check to partition read code path to return fast
when requested max offset is smaller than start offset indicating no
data should be returned.