dynamic partitioning: use probability of AddTask to root partition to estimate overall add task rate - #11699
Conversation
|
Claude finished @carlydf's task in 6m 52s —— View job Reviewed 9 files, 3 findings. |
| "google.golang.org/grpc/metadata" | ||
| ) | ||
|
|
||
| func TestEstimatedTasksAllPartitionsMetadata(t *testing.T) { |
f35ea5e to
92599a7
Compare
92599a7 to
095eb9d
Compare
| // PickWritePartition returns the task queue partition for adding an | ||
| // activity or workflow task and the estimated number of tasks added | ||
| // across all partitions per root task. The input is the name of the | ||
| // original task queue (with no partition info). When forwardedFrom |
There was a problem hiding this comment.
I just noticed we can remove the comment about forwardedFrom from here and the below comment, that's handed outside LoadBalancer now
| if partition.IsRoot() { | ||
| return partition, 1 | ||
| } | ||
| // probability of reaching root is 0, so root can't know how many tasks were added overall | ||
| return partition, 0 |
There was a problem hiding this comment.
I don't think it matters much, but semantically I think this should return something like, 0, partitionCount for root (and partition, 0 otherwise if you follow my suggestion above). like, arrange the code so this check goes in between setting partitionCount and pickWritePartitionByGap.
the presumption is that we do have a known number of partitions, and we're just pretending the random choice always comes up this way.
| if err != nil { | ||
| return taskQueue.RootPartition() | ||
| return taskQueue.RootPartition(), 1 | ||
| } |
There was a problem hiding this comment.
unrelated comment but could we move this nsName stuff into the else clause below? I forgot if there was a reason I didn't do that but it looks like we can.
| if gap <= 0 { // this partition is at or above cap | ||
| continue | ||
|
|
||
| // if rootProbability < 0.01, choose the root with p=0.01 |
There was a problem hiding this comment.
I'm debating if I like this or an alternative: use a different cap for part 0 than for the rest, and set that cap such that the probability works out to >= 1%. e.g. capfor0 = max(cap, total / 100 + decode(counts[0])). that avoids messing with floats and just uses one random value. what do you think?
There was a problem hiding this comment.
interested! but need to think about it a bit more
| return ctx | ||
| } | ||
| b := make([]byte, 8) | ||
| binary.LittleEndian.PutUint64(b, uint64(estimatedTasksAllPartitions)) |
There was a problem hiding this comment.
this is a number from 1 to 100.. uint64 is kind of overkill. how about varint just in case?
| } | ||
| b := make([]byte, 8) | ||
| binary.LittleEndian.PutUint64(b, uint64(estimatedTasksAllPartitions)) | ||
| return metadata.AppendToOutgoingContext(ctx, estimatedTasksAllPartitionsHeaderName, string(b)) |
There was a problem hiding this comment.
I was hoping this value could go in the clientpartitioncounts proto :( but I see it may be hard to arrange that. what do you think?
| if n := sm.currentWrite.Load(); n > 0 { | ||
| return n | ||
| } | ||
| return int32(sm.getWritePartitions()) |
There was a problem hiding this comment.
instead of doing a dc lookup here, could we just fall back to the old logic (use eTAP)?
| @@ -447,12 +447,11 @@ func (pm *taskQueuePartitionManagerImpl) signalPartitionScaler() { | |||
| if effectiveWrite == 0 { | |||
| effectiveWrite = max(1, pm.config.NumWritePartitions()) | |||
| } | |||
There was a problem hiding this comment.
only do all of this effectiveWrite stuff inside the if etap == 0 clause?
| "google.golang.org/grpc/metadata" | ||
| ) | ||
|
|
||
| func TestEstimatedTasksAllPartitionsMetadata(t *testing.T) { |
dd08b17 to
3cf2b61
Compare
|
Claude finished @carlydf's task in 4m 17s —— View job Reviewed 8 files, 4 findings. |
dnr
left a comment
There was a problem hiding this comment.
I think this is good even without fixing the stealing-from-last-partitions part, since it's only an issue for really really huge backlogs, and we can fix it before we see that. But you can fix that now if you want.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
…comment Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a9d22f8. Configure here.
a9d22f8 to
d38fe76
Compare
… estimate overall add task rate (#11699) ## What changed? - use probability of AddTask to root partition to estimate overall add task rate, instead of assuming uniform distribution - to enable this, change the AddTask loadbalancing to ensure that there is at least 1% chance of hitting the root, regardless of how much backlog the root has ## Why? A previous PR changed AddTask load-balancing from uniform random to backlog-aware ## How did you test it? - [x] built - [x] run locally and tested manually - [ ] covered by existing tests - [x] added new unit test(s) - [ ] added new functional test(s) ## Potential risks Changes AddTask computation and AddTask load balancing, but improves the estimation of the former, and we are ok with the tradeoff for the latter <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches AddTask write routing and dynamic partition scaling inputs; behavior changes when backlog-aware routing heavily favors child partitions, though a 1% root floor and fallback estimates limit worst-case drift. > > **Overview** > Backlog-aware write routing no longer assumes one root add implies uniform load on every partition. The matching client now returns an **estimated tasks across all partitions** when load-balancing picks the **root** partition, sends it on gRPC metadata (`etap`), and the root partition scaler uses that instead of multiplying by write partition count. > > **Write load balancing** enforces at least a **1% chance** of routing to the root (`writePartitionRootProbabilityFloor`) so sampling stays viable when the root backlog is full. When the root is chosen, the estimate is derived from gap-weighted routing (`randomRound(total/gap0)`); non-root picks carry zero estimate. Fallback paths (uniform random, forced partition via test hooks) still return `partitionCount` on root samples. > > **Scale manager** batches and wakes the partition scaler on **estimated** queue-wide task volume (`AddedTasks(estimatedTasksAllPartitions)`), with threshold `batchSize × currentWrite` once write count is known. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 1ee95df. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> (cherry picked from commit 2220587)
## What changed? Backports the 12 merged PRs labeled [release/1.32.0](https://github.com/temporalio/temporal/issues?q=repo%3Atemporalio%2Ftemporal+is%3Apr+label%3Arelease%2F1.32.0) that were not yet on `release/v1.32.x`, applied in `main` merge order. Also fixes mixedbrain test by bumping go version to 1.26.8 in mixedbrain folder (broken by #11928). | # | PR | Author | What / Why | |---|----|--------|------------| | 1 | [#11546](#11546) fix(batcher): scope deterministic request IDs to the batch job ID | @spkane31 | Two batch operations sending the same signal to the same workflow/run ID only delivered one — the request ID was hashed from workflow ID / run ID / signal name, so the second was de-duped in signal logic. Including the batch job ID in the hash keeps separate signals distinct. | | 2 | [#11642](#11642) Fix batch operations targeting paused executions | @fretz12 | A paused execution is still non-terminal, but the old `Running`-only filter silently skipped paused targets. Users issuing a batch terminate/signal/cancel reasonably expect it to apply to them. | | 3 | [#11666](#11666) Fix activity timeout regeneration after unpause | @fretz12 | Timeout tasks that fired while an activity was paused were discarded, but the activity's timer-task status still claimed they existed — preventing recreation after unpause and potentially making the timeout ineffective. Also fixes `ResetActivity` bypassing normal unpause handling, and running activities returning early without clearing paused state. | | 4 | [#11696](#11696) Treat missing Worker Deployment Version as deleted | @rkannan82 | Treats `NotFound` from the Version workflow delete update as success. When a Version workflow is already closed or its history is gone, `NotFound` blocked the Deployment workflow from removing its stale version reference. Fixes #11539. | | 5 | [#11725](#11725) Fix batch activity unpause visibility query scope | @fretz12 | The server replaced the caller's visibility query with the activity-type predicate, which could broaden the batch scope and unpause activities in workflows the caller never selected. | | 6 | [#11806](#11806) Fix activity operator command metric label collision | @fretz12 | WFA and SAA emitted the same Prometheus metric names (`activity_pause`/`unpause`/`reset`/`update_options`) with **different label sets**, causing descriptor registration failures and dropped metrics. Both now use the standard per-activity label schema and emit once per affected activity. **See "Backport adaptation" below.** | | 7 | [#11930](#11930) Prioritize worker versioning workflows | @ShahabT | Worker versioning workflows share the per-namespace worker task queue with other system workflows. Raising their initial workflow-task priority above default keeps versioning operations from queueing behind default-priority work. | | 8 | [#11910](#11910) [Visibility] Fix legacy visibility query converter | @rodrigozhou | Fixes to the legacy visibility query converter for both SQL and Elasticsearch. | | 9 | [#11699](#11699) dynamic partitioning: use probability of AddTask to root partition to estimate overall add task rate | @carlydf | A previous PR changed AddTask load-balancing from uniform random to backlog-aware, invalidating the uniform-distribution assumption behind the add-task-rate estimate. Now estimates from the probability of AddTask hitting the root partition, and guarantees the root retains at least a 1% chance of selection regardless of its backlog. | | 10 | [#11935](#11935) [Visibility] Enable unified query converter by default | @rodrigozhou | Flips the default now that the unified query converter is stable and ready to replace the old one. Depends on #11910 and #11801. | ## How did you test it? - [x] built — `go build ./...` - [x] `make fmt` — no changes - [x] `go vet -tags disable_grpc_modules,test_dep ./tests/...` — clean - [x] `make GOLANGCI_LINT_FIX=false GOLANGCI_LINT_BASE_REV=origin/release/v1.32.x lint-code` — **0 issues** - [x] covered by existing tests — 21 affected packages, **20 pass** --------- Co-authored-by: Sean Kane <spkane31@gmail.com> Co-authored-by: Fred Tzeng <41805201+fretz12@users.noreply.github.com> Co-authored-by: Kannan <rkannan82@users.noreply.github.com> Co-authored-by: Shahab Tajik <shahab@temporal.io> Co-authored-by: Rodrigo Zhou <rodrigo.zhou@temporal.io> Co-authored-by: Carly de Frondeville <carly.defrondeville@temporal.io> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Vladyslav Simonenko <vlad.simonenko@temporal.io>

What changed?
Why?
A previous PR changed AddTask load-balancing from uniform random to backlog-aware
How did you test it?
Potential risks
Changes AddTask computation and AddTask load balancing, but improves the estimation of the former, and we are ok with the tradeoff for the latter
Note
Medium Risk
Touches AddTask write routing and dynamic partition scaling inputs; behavior changes when backlog-aware routing heavily favors child partitions, though a 1% root floor and fallback estimates limit worst-case drift.
Overview
Backlog-aware write routing no longer assumes one root add implies uniform load on every partition. The matching client now returns an estimated tasks across all partitions when load-balancing picks the root partition, sends it on gRPC metadata (
etap), and the root partition scaler uses that instead of multiplying by write partition count.Write load balancing enforces at least a 1% chance of routing to the root (
writePartitionRootProbabilityFloor) so sampling stays viable when the root backlog is full. When the root is chosen, the estimate is derived from gap-weighted routing (randomRound(total/gap0)); non-root picks carry zero estimate. Fallback paths (uniform random, forced partition via test hooks) still returnpartitionCounton root samples.Scale manager batches and wakes the partition scaler on estimated queue-wide task volume (
AddedTasks(estimatedTasksAllPartitions)), with thresholdbatchSize × currentWriteonce write count is known.Reviewed by Cursor Bugbot for commit 1ee95df. Bugbot is set up for automated code reviews on this repo. Configure here.