-
Notifications
You must be signed in to change notification settings - Fork 30
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: doCommitOffsets of PulsarKafkaConsumer in version 2.11.0 #49
base: master
Are you sure you want to change the base?
Conversation
…eIdImpl in doCommitOffsets method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but needs a test that reproes original problem
@eolivelli @dlg99 added a unit test for commitSync, specifically on how acknowledgeCumulativeAsync is called inside doCommitOffsets. Let me know if there is any thing else required. |
@dlg99, any idea when can this be merged if everything is good? Also, can a patch version be released with this fix ? |
I support to merge this patch. @dlg99 @eolivelli could you give another look? |
Fixes #48
Master Issue: #48
Motivation
doCommitOffsets method in Pulsar Kafka Client version 2.11.0 is not working for partitioned topics. This does not allows acking messages.
Modifications
In doCommitOffsets PulsarKafkaConsumer.java creating TopicMessageIdImpl with the correct partition name
Verifying this change
This change is already covered by existing tests, provided we create it as a partitioned topic
Does this pull request potentially affect one of the following parts:
Documentation