Batch operation rate limit #4941
Merged
Conversation
This file contains 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
stephanos
force-pushed
the
batch-operation-rate-limit
branch
from
4ffb894
to
20977ea
Compare
stephanos
commented
Oct 5, 2023
| @@ -45,7 +45,6 @@ type activitiesSuite struct { | |||
| testsuite.WorkflowTestSuite | |||
|
|
|||
| controller *gomock.Controller | |||
| env *testsuite.TestWorkflowEnvironment | |||
stephanos
force-pushed
the
batch-operation-rate-limit
branch
2 times, most recently
from
fca2cc2
to
8fed2d0
Compare
stephanos
force-pushed
the
batch-operation-rate-limit
branch
from
8fed2d0
to
7c05712
Compare
stephanos
force-pushed
the
batch-operation-rate-limit
branch
3 times, most recently
from
efbf5b5
to
7fddbfe
Compare
stephanos
commented
Oct 30, 2023
Comment on lines
+205
to
+210
| func (a *activities) getOperationRPS(requestedRPS float64) float64 { | ||
| maxRPS := float64(a.rps(a.namespace.String())) | ||
| if requestedRPS <= 0 || requestedRPS > maxRPS { | ||
| return maxRPS | ||
| } | ||
| return rps | ||
| return requestedRPS |
There was a problem hiding this comment.
This is the main change; all others are just cosmetic (making fields private, fixing up formatting and comments etc.)
stephanos
commented
Oct 30, 2023
| @@ -43,7 +43,7 @@ require ( | |||
| go.opentelemetry.io/otel/metric v1.19.0 | |||
| go.opentelemetry.io/otel/sdk v1.19.0 | |||
| go.opentelemetry.io/otel/sdk/metric v1.19.0 | |||
| go.temporal.io/api v1.24.1-0.20231003165936-bb03061759c8 | |||
| go.temporal.io/api v1.25.0 | |||
There was a problem hiding this comment.
Upgraded to new API version to pull in the max_operations_per_second field.
stephanos
force-pushed
the
batch-operation-rate-limit
branch
4 times, most recently
from
0734e72
to
575c3ce
Compare
stephanos
commented
Oct 30, 2023
| RPS int | ||
| // RPS sets the requests-per-second limit for the batch. | ||
| // The default (and max) is defined by `worker.BatcherRPS` in the dynamic config. | ||
| RPS float64 |
There was a problem hiding this comment.
This was effectively unused before.
stephanos
force-pushed
the
batch-operation-rate-limit
branch
from
575c3ce
to
739e557
Compare
stephanos
commented
Oct 30, 2023
There was a problem hiding this comment.
why are there so many deletions? 😕
yiminc
approved these changes
Oct 30, 2023
stephanos
added a commit
that referenced
this pull request
Feb 21, 2024
## What changed? Adding back one line that was accidentally removed from #4941 after it was merged. Line added originally: https://github.com/temporalio/temporal/pull/4941/files#diff-c9a59445e489bd70346630f2773ad69e1723b2298fadbae50ff5c1a424917e1cR3468 ## Why? <!-- Tell your future self why have you made these changes --> ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Is hotfix candidate? No. The code will still work since we ignore it when `requestedRPS` is zero: ``` if requestedRPS <= 0 || requestedRPS > maxRPS { return maxRPS } ```
tdeebswihart
pushed a commit
that referenced
this pull request
Feb 21, 2024
## What changed? Adding back one line that was accidentally removed from #4941 after it was merged. Line added originally: https://github.com/temporalio/temporal/pull/4941/files#diff-c9a59445e489bd70346630f2773ad69e1723b2298fadbae50ff5c1a424917e1cR3468 ## Why? <!-- Tell your future self why have you made these changes --> ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Is hotfix candidate? No. The code will still work since we ignore it when `requestedRPS` is zero: ``` if requestedRPS <= 0 || requestedRPS > maxRPS { return maxRPS } ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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?
Allow rate limiting a batch operation. Fixes #4926.
Why?
Batch operations are run server side and may effect millions of executions, this in turn may overload workers and disrupt normal operations.
How did you test it?
I started the Server locally and initiated a batch operation from the CLI (CLI changes be found here):
Potential risks
Rate limiting to be incorrect and slow down processing or disrupt cluster.
Is hotfix candidate?
No