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

Do a final commit on end consumer group generation for immediate commits #715

Merged
merged 2 commits into from
Jan 21, 2022

Conversation

stevevls
Copy link
Contributor

This is a WIP and isn't ready to commit until I put a unit test together. 😄

This PR adds a final commit upon exiting the consumer group when using the synchronous commit mode. Otherwise, it's possible that we lose commits on generation end (or other events that initiate the end of a generation such as closing the reader).

Fixes #711

@rhansen2
Copy link
Collaborator

Possible related to #772

@stevevls
Copy link
Contributor Author

Good call. This definitely could be related since they are not setting the CommitInterval. I think most users (myself included) don't realize that the commit loop has to be explicitly enabled.

@rhansen2
Copy link
Collaborator

rhansen2 commented Nov 15, 2021

From https://docs.confluent.io/platform/current/clients/consumer.html

Offset commit failures are merely annoying if the following commits succeed since they won’t actually result in duplicate reads. However, if the last commit fails before a rebalance occurs or before the consumer is shut down, then offsets will be reset to the last commit and you will likely see duplicates. A common pattern is therefore to combine async commits in the poll loop with sync commits on rebalances or shut down. Committing on close is straightforward, but you need a way to hook into rebalances.

Sounds like this would be a nice addition to the Reader.

@stevevls Are you interested in continuing this PR and adding test? If not I could give it a crack.

@derekbassett
Copy link
Contributor

derekbassett commented Dec 3, 2021

If this PR is not ready for review might make sense to make it a Draft PR. @rhansen2 I think this makes sense for you to take over since it's been several days.

@derekbassett derekbassett assigned rhansen2 and unassigned derekbassett Dec 3, 2021
@derekbassett derekbassett requested review from derekbassett and removed request for derekbassett December 3, 2021 18:44
@rhansen2 rhansen2 force-pushed the fix-711 branch 2 times, most recently from 1cdbde1 to 0ddaf4f Compare January 17, 2022 23:02
Copy link
Contributor

@achille-roussel achille-roussel left a comment

Choose a reason for hiding this comment

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

🚢 it!

@rhansen2 rhansen2 merged commit 382e96d into main Jan 21, 2022
@rhansen2 rhansen2 deleted the fix-711 branch January 21, 2022 18:47
@stevevls
Copy link
Contributor Author

Thanks for landing this team! 🙌 I'm sorry I didn't see the mention for whether I wanted to keep working on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Question: starting offsets - do they work with consumer groups?
4 participants