Attach RateLimiter using UpdateTaskQueueConfig api configration#8058
Merged
sivagirish81 merged 25 commits intotemporalio:mainfrom Aug 6, 2025
Merged
Attach RateLimiter using UpdateTaskQueueConfig api configration#8058sivagirish81 merged 25 commits intotemporalio:mainfrom
sivagirish81 merged 25 commits intotemporalio:mainfrom
Conversation
sivagirish81
commented
Jul 16, 2025
This comment was marked as resolved.
This comment was marked as resolved.
sivagirish81
commented
Jul 18, 2025
This comment was marked as resolved.
This comment was marked as resolved.
dnr
reviewed
Jul 22, 2025
sivagirish81
commented
Jul 22, 2025
sivagirish81
commented
Jul 22, 2025
sivagirish81
commented
Jul 22, 2025
stephanos
reviewed
Jul 22, 2025
stephanos
reviewed
Jul 22, 2025
stephanos
reviewed
Jul 22, 2025
stephanos
reviewed
Jul 22, 2025
tests/task_queue_test.go
Outdated
|
|
||
| workflowFn := func(ctx workflow.Context) error { | ||
| ao := workflow.ActivityOptions{ | ||
| TaskQueue: activityTaskQueue, // Route activities to a dedicated task queue named `activityTaskQueue` |
Contributor
There was a problem hiding this comment.
Maybe reword the comment to mention why you're doing this instead.
stephanos
reviewed
Jul 22, 2025
tests/task_queue_test.go
Outdated
| func (s *TaskQueueSuite) runTestTaskQueueAPIRateLimitOverridesWorkerLimit(apiRPS float32, taskCount int, minGap time.Duration, maxGap time.Duration, activityTaskQueue string) { | ||
| // Set the burst as 1 to make sure not more than 1 task get's acknowledged at a time. | ||
| // Helps observe the backlog drain more easily. | ||
| s.OverrideDynamicConfig(dynamicconfig.MatchingMinTaskThrottlingBurstSize, 1) |
Contributor
There was a problem hiding this comment.
Can you set it to 0 to avoid the special case at the bottom of the test?
Contributor
Author
There was a problem hiding this comment.
Burst is overriden regardless of the value we set here at time of ratelimitupdate.
minTaskThrottlingBurstSize := r.config.MinTaskThrottlingBurstSize()
if burst < minTaskThrottlingBurstSize {
burst = minTaskThrottlingBurstSize
}
So we will need to enforce the special case at the bottom of the test - ignore the gap between the first and the second task and check for the gap between the remaining tasks.
stephanos
reviewed
Jul 22, 2025
dnr
reviewed
Jul 23, 2025
Fixed unit test and naming convention for locked functions.
dnr
reviewed
Jul 24, 2025
dnr
reviewed
Jul 25, 2025
sivagirish81
commented
Jul 30, 2025
sivagirish81
commented
Jul 30, 2025
5 tasks
dnr
reviewed
Aug 1, 2025
Contributor
dnr
left a comment
dnr
left a comment
There was a problem hiding this comment.
alright, a few small comments. the partition count stuff could be handled in the next pr. what do you think about the others?
dnr
reviewed
Aug 6, 2025
tests/task_queue_test.go
Outdated
|
|
||
| // Start the activity worker | ||
| activityWorker := worker.New(s.SdkClient(), activityTaskQueue, worker.Options{ | ||
| // Setting rate limit at worker level |
Contributor
There was a problem hiding this comment.
Suggested change
| // Setting rate limit at worker level | |
| // Setting rate limit at worker level (this will be ignored in favor of the limit set through the api) |
tests/task_queue_test.go
Outdated
| defer wfWorker.Stop() | ||
|
|
||
| // Launch workflows | ||
| for i := 0; i < taskCount; i++ { |
Contributor
There was a problem hiding this comment.
Suggested change
| for i := 0; i < taskCount; i++ { | |
| for i := range taskCount { |
dnr
approved these changes
Aug 6, 2025
This was referenced Aug 6, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed?
Why?
How did you test it?
Potential risks
N/A