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

KafkaJSNumberOfRetriesExceeded should reflect the original retry-ability #1274

Merged
merged 2 commits into from
Jan 27, 2022
Merged

KafkaJSNumberOfRetriesExceeded should reflect the original retry-ability #1274

merged 2 commits into from
Jan 27, 2022

Conversation

JanGroot
Copy link

The KafkaJSNumberOfRetriesExceed now reflects if the original error was retryable.
If so: The onCrash will restart the consumer (regarding shouldRestartOnFailure)
If not: The onCrash will not.

closes #1271

@Nevon
Copy link
Collaborator

Nevon commented Jan 26, 2022

The intention is that the consumer should always retry if the reason for the crash was due to some temporary error that we could recover from by tossing away the current state and restarting. That's why we restart if the error is either retriable or KafkaJSNumberOfRetriesExceeded.

So the intention of this change, to make it so that we restart only if the error that caused us to run out of retries was a retriable one, makes sense. It's just unfortunate that the way to do it is to make KafkaJSNumberOfRetriesExceeded itself potentially retriable. At least to me, it seems unintuitive that an error saying that we're out of retries is retriable. It's kind of repurposing the property to mean something different than what it means for all other errors.

What do you think about keeping KafkaJSNumberOfRetriesExceeded as non-retriable, but change the restart check to e.retriable || (e.name === 'KafkaJSNumberOfRetriesExceeded' && e.originalError.retriable)?

@JanGroot
Copy link
Author

Good point, above anything, it should be clear what the intention is. I have another idea, I'll update the PR.

If an error is not retriable, reject with a KafkaJSNonRetriableError.

closes #1271

Signed-off-by: Jan Groot <jan.groot@abnamroclearing.com>
@JanGroot
Copy link
Author

New and more clear approach:
If the error was not retriable to begin with, reject with KafkaJSNonRetriableError.

closes #1271

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.

NonRetryable errors cause the consumer to restart, causing a loop.
2 participants