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.

Sign up for GitHub

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

Jump to bottom

OffsetFetchResponse and OffsetCommitRequest leaderEpoch is not working as expected #2694

Closed
kabochya opened this issue Oct 26, 2023 · 2 comments · Fixed by #2705
Closed

OffsetFetchResponse and OffsetCommitRequest leaderEpoch is not working as expected #2694

kabochya opened this issue Oct 26, 2023 · 2 comments · Fixed by #2705
Assignees
@dnwe
Labels
bug

Comments

@kabochya
Copy link

kabochya commented Oct 26, 2023 •
edited
Loading

Description

When using PartitionOffsetManager to reset consumer group offsets, we noticed that other consumers kept getting offsetEpoch = 0 in FetchRequests

I think the culprit is that we are not setting
https://github.com/IBM/sarama/blob/main/offset_fetch_response.go#L7
to -1 by default

in newPartitionOffsetManager we invoke fetchInitialOffset which populates the pom.leaderEpoch. However, we use Version 1 for the OffsetFetchRequest: https://github.com/IBM/sarama/blob/main/offset_manager.go#L157

Since we don't set the leaderEpoch for Version < 5 (ref), PartitionOffsetManager always sets leaderEpoch = 0, which when doing a resetOffset and commit later on, will get committed with the incorrect leaderEpoch (ref)

Versions
Sarama Kafka Go
1.41.1 2.7.1 1.21
Configuration

n/a

Additional Context

Not sure if this could be fixed by having version handling done better in fetchInitialOffsets
@dnwe dnwe self-assigned this Nov 2, 2023
@dnwe dnwe added the bug label Nov 2, 2023
@dnwe
Copy link
Collaborator

dnwe commented Nov 2, 2023

Thanks this is a good catch! Will work on a fix

dnwe added a commit that referenced this issue Nov 2, 2023
@dnwe
fix: make fetchInitialOffset use correct protocol
ad5b5ab 
The `fetchInitialOffset` func in offsetManager was hardcoded to send a
Version 1 OffsetFetchRequest rather than sending the appropriate version
based on the config KafkaVersion. As discussed in #2694 this meant that
LeaderEpoch was always being decoded as the default value '0' (because
it was only returned in Version >= 5 OffsetFetchRequest). However, other
areas of the offsetManager code were sending the newer protocol
versions, so for example the OffsetCommitRequest would include a leader
epoch value of 0 rather than an accurate one.

Correct this bug by sending the correct protocol version in
fetchInitialOffset and also ensure we default to `-1` when we decode an
OffsetFetchResponse of a Version < 5

Fixes #2694

Signed-off-by: Dominic Evans <dominic.evans@uk.ibm.com>
@dnwe dnwe mentioned this issue Nov 2, 2023
Merged
@dnwe dnwe closed this as completed in #2705 Nov 3, 2023
dnwe added a commit that referenced this issue Nov 3, 2023
@dnwe
fix: make fetchInitialOffset use correct protocol (#2705)
27710af 
The `fetchInitialOffset` func in offsetManager was hardcoded to send a
Version 1 OffsetFetchRequest rather than sending the appropriate version
based on the config KafkaVersion. As discussed in #2694 this meant that
LeaderEpoch was always being decoded as the default value '0' (because
it was only returned in Version >= 5 OffsetFetchRequest). However, other
areas of the offsetManager code were sending the newer protocol
versions, so for example the OffsetCommitRequest would include a leader
epoch value of 0 rather than an accurate one.

Correct this bug by sending the correct protocol version in
fetchInitialOffset and also ensure we default to `-1` when we decode an
OffsetFetchResponse of a Version < 5

Fixes #2694

Signed-off-by: Dominic Evans <dominic.evans@uk.ibm.com>
@dnwe
Copy link
Collaborator

dnwe commented Nov 3, 2023

@kabochya are you able to test against github.com/IBM/sarama@27710af and confirm that you're happy with the fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dnwe dnwe

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: make fetchInitialOffset use correct protocol IBM/sarama
2 participants
@kabochya @dnwe