-
Notifications
You must be signed in to change notification settings - Fork 874
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
Prioritize task loading persistence requests #3217
Conversation
51ba63a
to
b8ce0a6
Compare
) | ||
|
||
func NewPriorityRateLimiter( | ||
namespaceMaxQPS PersistenceNamespaceMaxQps, | ||
hostMaxQPS PersistenceMaxQps, | ||
requestPriorityFn quotas.RequestPriorityFn, |
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.
Taking it as an input so the logic can be substitute if needed.
// dynamic config. | ||
p.ConstructHistoryTaskAPI("GetHistoryTasks", tasks.CategoryTransfer): 2, | ||
p.ConstructHistoryTaskAPI("GetHistoryTasks", tasks.CategoryTimer): 2, | ||
p.ConstructHistoryTaskAPI("GetHistoryTasks", tasks.CategoryVisibility): 2, |
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.
Excluded replication get history tasks from being prioritized.
// persistence metrics client. For now, it's only used by rate | ||
// limit client, and we don't really care about the actual value | ||
// returned, as long as they are different from each task category. | ||
func ConstructHistoryTaskAPI( |
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.
persistence retry/metrics/ratelimit client should be moved to persistence/client
package. Then this function doesn't have to be exported.
@@ -42,6 +42,10 @@ import ( | |||
"go.uber.org/fx" | |||
) | |||
|
|||
const ( | |||
timerQueuePersistenceMaxRPSRatio = 0.3 |
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.
the actual usage is much low than this, so it should be quite safe.
What changed?
Why?
How did you test it?
Potential risks
Is hotfix candidate?