-
Notifications
You must be signed in to change notification settings - Fork 1
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
[ID-98] Rate limits #139
[ID-98] Rate limits #139
Conversation
…s was applied to all environments
When you test the changes you first need to:
|
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.
LGTM
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 @petyos, this is really great work! I think this implementation is really well structured and easy to follow. I did leave a couple of comments below talking about some edge cases that may come up if a thread or instance crashes. I don't believe that either one is critically urgent enough to stop this from being merged, but let me know if you have any questions or thoughts on them. Thanks again!
if queue.Status != "ready" { | ||
q.logger.Infof("the queue is not ready but %s", queue.Status) | ||
queueAvailable = false | ||
return nil | ||
} |
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.
One thing to consider here is what happens if the thread dies while processing the queue. In this case, it seems to me that the queue would be stuck in an unavailable state and would never automatically recover. The way I handled a similar locking situation on the Groups BB in the past was to assign a timestamp when the lock is set and check for potential timeout scenarios. To do this, if the status is not "ready" we would also check if the timestamp is older than some configurable timeout period, and if it is we would override the status and begin processing the queue anyway. Let me know if you have any questions or any other thoughts on how to handle this case.
func (q queueLogic) start() { | ||
q.logger.Info("queueLogic start") | ||
|
||
q.processQueue() |
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.
I think there may be an issue here if the queue is locked when the instance first starts. In this case, it looks to me like the instance will never attempt to process the queue unless the specific instance receives a new message through the API. In normal functioning I think that is ok, but if some of the instances crash I think we could end up in a state where no instances are actively processing the timed items in the queue. One potential solution could be to periodically retry processing the queue until it is able to acquire the lock.
Hi @mdryankov , @shurwit , thanks for your feedback. I am merging it. @shurwit I've opened a separate task for covering your feedback here - #140 |
Description
**Resolves #98
Here are the highlihts of this PR:
Review Time Estimate
Please give your idea of how soon this pull request needs to be reviewed by selecting one of the options below. This can be based on the criticality of the issue at hand and/or other relevant factors.
Type of changes
Please select a relevant option: