-
Notifications
You must be signed in to change notification settings - Fork 800
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
Rate limit task processing requests guarded by feature flag #5804
Conversation
Codecov Report
Additional details and impacted files
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Pull Request Test Coverage Report for Build 018e65a4-d74b-470b-8e14-ee4b052c9737Details
💛 - Coveralls |
@@ -198,8 +202,10 @@ func (t *transferActiveTaskExecutor) processActivityTask( | |||
// the rest of logic is making RPC call, which takes time. | |||
release(nil) | |||
|
|||
// Ratelimiting is not done. This is only to count the number of requests via metrics | |||
t.wfIDCache.AllowInternal(task.DomainID, task.WorkflowID) | |||
// Rate limiting task processing requests |
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.
What is the reason to apply this for activity and decision tasks only? Are you planning to expand to other task types later on?
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.
A lot of processing boils down to these two task types and they are easier to reason about when we ratelimit users. But it is possible to expand if and when needed
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.
Child workflow related tasks are also common and cause hot shards. I recommend including them in the scope
What changed?
Task processing requests will be rate limited if enabled by domain level feature flag
Why?
Introducing wf-id based rate limits for task processing
How did you test it?
Unit tests
Potential risks
None for now since the rate limiting of task processing is guarded by feature flag.
Release notes
Documentation Changes