-
Notifications
You must be signed in to change notification settings - Fork 334
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
feat: terminateVisibilityTimeout = number to customise the timeout #520
feat: terminateVisibilityTimeout = number to customise the timeout #520
Conversation
CLA Assistant Lite bot CLA CHECK All Contributors have signed the CLA |
I have read the CLA Document and I hereby sign the CLA |
524d1cf
to
a73942f
Compare
src/consumer.ts
Outdated
if (this.terminateVisibilityTimeout) { | ||
await this.changeVisibilityTimeout(message, 0); | ||
if (this.terminateVisibilityTimeout !== false) { | ||
const timeout = this.terminateVisibilityTimeout === true ? 0 : this.terminateVisibilityTimeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, this makes sense. I am currently thinking if it makes sense to add a new option though (terminateVisibilityTimeoutMs
) rather than this logic and then just default that to 0 and allow it to be changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review! To be honest I'm on the fence too. If we reuse the terminateVisibilityTimeout
option, the naming is a bit confusing; if we add a terminateVisibilityTimeoutMs
, without looking at their types, there will appear to be a timeout and a timeout in milliseconds at the same time, and I'm not sure what should happen if terminateVisibilityTimeout == false
but terminateVisibilityTimeoutMs
is set.
I'm happy to leave the decision to you. I can update the code and docs if you opt for a new option in the end.
PS: Forgot to npm run format
. Just pushed :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that makes sense too, I think I'll just merge this into canary for now and think on it, if I don't decide against it by Wednesday, I'll push it up to main.
Thanks a lot for the contribution!
This allows users to introduce a delay before the message can be received again. In some cases this can improve the overall success rate.
a73942f
to
092c3b9
Compare
Resolves #523
Description:
This allows users to introduce a delay before the message can be received again. In some cases this can improve the overall success rate.
Type of change:
Why is this change required?:
When tasks are not time critical, a small delay can mitigate some intermittent errors, or give supervisors like K8s a little time to kill broken consumers.
Code changes:
terminateVisibilityTimeout
option has been updated to accept custom visibility timeouts.I reused the option as I thought it was the most non-intrusive way to introduce this feature. If it appears to be confusing, I can also add a new option. But since the underlying SQS command is the same, I think it's ok.
Checklist: