search

zio / zio-kafka

A Kafka client for ZIO and ZIO Streams
https://zio.dev/zio-kafka
Apache License 2.0
337 stars 138 forks source link

retry Consumer.CommitTimeout exception #1140

Closed myazinn closed 9 months ago

myazinn commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

svroonland commented 9 months ago

See also #1126

myazinn commented 9 months ago

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 commented 9 months ago

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