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

retry Consumer.CommitTimeout exception #1140

Merged
merged 1 commit into from
Dec 20, 2023

Conversation

myazinn
Copy link
Contributor

@myazinn myazinn commented Dec 19, 2023

Though this is a debatable change, I think it's not obious that Consumer.CommitTimeout exception is not retrieable when using Offset#commitOrRetry or OffsetBatch#commitOrRetry.

@erikvanoosten
Copy link
Collaborator

erikvanoosten commented Dec 20, 2023

Thanks for your PR @myazinn . I am afraid we cannot accept it.

The purpose of a time out is to abort an operation that is taking too long and is therefor unlikely to succeed. Sure, you can retry this, but it should never be tried indefinitely, RetriableCommitFailedException exceptions can be retried indefinitely.

In short, zio-kafka cannot decide how often a commit timeout should be retried. Therefore, we have to delegate this to the user. The user can either increase the timeout, or do a retry in their code.

@erikvanoosten
Copy link
Collaborator

Okay, I am re-reading the code. And now I see that there is also the user provided policy... Have to think more about this 🙂

@erikvanoosten
Copy link
Collaborator

@myazinn Can you tell a bit about what motivated you to write this PR?

@svroonland
Copy link
Collaborator

See also #1126

@myazinn
Copy link
Contributor Author

myazinn commented Dec 20, 2023

Hi @erikvanoosten

I am afraid we cannot accept it.

If that's the case, no worries, we'll add it on our side :) I just thought it was overlooked

Can you tell a bit about what motivated you to write this PR?

One of our services has been failing from time to time (once or twice in a month) with timeouts after we updated zio-kafka several months ago. It's unclear why it fails since a lot of others work fine, but usually it timeouts a few times and then works fine. I was surprised too see that even though we use commitOrRetry, it doesn't retry anything and just restarts

@erikvanoosten erikvanoosten merged commit b741f60 into zio:master Dec 20, 2023
27 checks passed
@erikvanoosten
Copy link
Collaborator

My earlier reasoning didn't make sense. Thanks for your PR @myazinn.

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