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

Maintain metadata in the producer even when retries are disabled. #1189

Merged

Conversation

stevevls
Copy link
Contributor

@stevevls stevevls commented Oct 8, 2018

Some applications may wish to disable retries so that they can implement custom
retry logic. As currently written, though, setting Config.Producer.Retry.Max to
zero also disables the mechanism by which the producer updates its metadata in
response to an error. If the producer gets an error such as
ErrNotLeaderForPartition or an io.EOF, it will never recover...even in response
to background metadata updates. This commit resolves that issue by ensuring
that the partition producers still update the leader and corresponding input
channel even when retries are turned off.

Steve van Loben Sels added 2 commits October 8, 2018 16:16
Some applications may wish to disable retries so that they can implement custom
retry logic.  As currently written, though, setting Config.Producer.Retry.Max to
zero also disables the mechanism by which the producer updates its metadata in
response to an error.  If the producer gets an error such as
ErrNotLeaderForPartition or an io.EOF, it will never recover...even in response
to background metadata updates.  This commit resolves that issue by ensuring
that the partition producers still update the leader and corresponding input
channel even when retries are turned off.
@stevevls
Copy link
Contributor Author

stevevls commented Oct 9, 2018

I imagine that there aren't very many folks who set Config.Producer.Retry.Max to zero, but I'm actually working on a couple apps with very specific retry requirements at the moment. 😄 This patch addresses keeping the partitionProducer's brokerProducer reference up to date in a minimally-invasive way. I had initially attempted to splice the functionality into the retry high/low watermark code, but it got pretty complex to reason about the different scenarios, and I abandoned that in favor of this code. Any feedback or suggestions on how to improve this patch are much appreciated.

Copy link

@sam-obeid sam-obeid left a comment

Choose a reason for hiding this comment

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

This sounds reasonable! Change looks good and tests pass!

@bai bai merged commit 45ea73e into IBM:master Feb 15, 2019
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.

3 participants