-
Notifications
You must be signed in to change notification settings - Fork 212
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
fix: Bug: Slack rate limit #1386
Conversation
Hi @alikonhz thanks for the willing to contribute to Memphis, it will be carefully reviewed and we will update over here as soon as we can |
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.
Hi @alikonhz thanks for this amazing PR it would help Memphis a lot.
I left some comments.
Also, you will notice that you have a few conflicts to resolve
…e sendInternalAccountMsgWithEcho when saving notification to the queue
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.
Hi @alikonhz it is very closed to be completed just a few last comments
…notifications filter subject for the notification consumer. Add time.sleep when Slack returns rate limit error
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.
@alikonhz Last improvements that we need are in the comments. Thanks
… In case of rate limit error send NACK with RetryAfter as a delay
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.
Another small thing please
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 @alikonhz for the amazing work, approved, merged and will be released on the next version
This is my attempt to fix the issue with Slack notifications being rate limited.
Fixes issue #919