Nevon
commented
2 years ago This behavior would have changed with #1274, which provides more context. The intention of the mechanism is to allow a consumer to restart if the reason for the crash was a retriable error. Previously, if any error was thrown from inside a retry block, it would end up throwing a KafkaJSNumberOfRetriesExceeded error, which was always considered retriable. Now, if the original error is not retriable, it instead throws a KafkaJSNonRetriableError - essentially it's reflecting the retriability of the original cause. I would say it's a bug fix, since a non-retriable error magically became retriable just because it happened to be thrown from inside a retrier.
await retry(async () => {
throw new TypeError('💣') // This would have resulted in a restart, even though it would never work
})
throw new TypeError('🐍') // This would *not* have resulted in a restart, because it wasn't wrapped in a retrierNon-retriable errors never led to a restart - unless they happened to be thrown from inside a retrier. This doesn't really make sense to me. The new approach is consistent, whether the error comes from a retrier or not.
await retry(async () => {
throw new TypeError('💣') // This will now not cause a restart
})
await retry(async () => {
throw new KafkaJSConnectionError('🔌') // This is retriable and will eventually cause a restart
})Is it definitely intended that restartOnFailure no longer allows customisation of behaviour for KafkaJSNumberOfRetriesExceeded and will this change be retained?
restartOnFailure still controls the behavior for retriable errors, whether they are coming from inside a retrier or not. It does not control the behavior of non-retriable errors, however. For example, if there's a TypeError being thrown from somewhere, it will cause a crash and not a restart, whether it's in a retrier or not.
I believe it's saying specifically that consumers will restart on KafkaJSNumberOfRetriesExceeded but that is no longer the case
It should be updated to say that consumers will restart on retriable errors.
I'd assumed that previously, KafkaJSNumberOfRetriesExceeded would lead to a restart because a restart would potentially resolve whatever was causing the retries to fail. Is that not the case?
Precisely. If the error is retriable, but we ran out of attempts, the assumption is that if we throw away current state and restart, the issue may resolve itself. If the error is not retriable, then the assumption is that restarting won't help. See #69 for the original feature request and related pull request.
is it the case that any logic relating to restarting because of KafkaJSNumberOfRetriesExceeded would now be expected to be in a consumer crash listener?
If you want to restart the consumer on any error, whether it's a transient network error or a TypeError, then at the moment I don't see any other way than listening for the crash event and manually recreating the consumer. But then I would ask what the point would be of restarting on an error that won't be solved by restarting.
rkclark
KiriLucas
FelipeEmerim
uwburn
Describe the bug Following changes in v1.16.0 we've experienced a breaking change within our application relating to https://github.com/tulios/kafkajs/pull/1274. I'm opening this issue to: 1) check my understanding of what is happening 2) check whether the v1.16.0 approach is likely to change 3) determine what changes I may need to make in our application
1.15.0 / old behaviour
At v1.15.0 and below,
KafkaJSNumberOfRetriesExceededwas classed as being an error that would cause the consumer to restart. Potentially, this could lead to an infinite restart loop. However, my understanding was that therestartOnFailureoption was available to allow users to customise that behaviour to the needs of the application, per the docs here: https://kafka.js.org/docs/configuration#restartonfailure.This is what we have done in our application. In our
restartOnFailure, we specifically watch forKafkaJSNumberOfRetriesExceededand we only allow the consumer to restart a specific number of times based on this error before we then gracefully shutdown and exit.1.16.0 / new behaviour
In v1.16.0 this no longer works.
KafkaJSNumberOfRetriesExceededleads directly to a consumer crash and does not trigger therestartOnFailuremethod. Based on this I have the following questions:restartOnFailureno longer allows customisation of behaviour forKafkaJSNumberOfRetriesExceededand will this change be retained? Don't want to modify our consumers to work with v1.16.0 if it might change again in the near future!restartOnFailurestill valid? https://kafka.js.org/docs/configuration#restartonfailure I believe it's saying specifically that consumers will restart onKafkaJSNumberOfRetriesExceededbut that is no longer the case 🤔KafkaJSNumberOfRetriesExceededwould lead to a restart because a restart would potentially resolve whatever was causing the retries to fail. Is that not the case? What is the benefit of a restart compared to retry?KafkaJSNumberOfRetriesExceededwould now be expected to be in a consumer crash listener? i.e. we'd need to "manually" restart rather than using the consumer's built in restart/restartOnFailure behaviour.Thanks for the awesome library and I hope the above makes sense 🙏
Environment: