Set task poller type for scalable task pollers#2248
Set task poller type for scalable task pollers#2248kritibehl wants to merge 1 commit intotemporalio:mainfrom
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review.
Tip: disable this comment in your organization's Code Review settings.
|
Friendly follow-up: this fixes #2235 by wiring workflow poller type through scalable task poller construction and adds regression coverage for the assignment path. Happy to adjust if you’d prefer a different shape for the fix. |
| opts.executionParameters.Logger, | ||
| params.NexusTaskPollerBehavior, | ||
| params.serverSupportsAutoscaling), | ||
| params.NexusTaskPollerBehavior, ""), |
There was a problem hiding this comment.
This should pass PollerTypeNexusTask it seems
There was a problem hiding this comment.
Fixed in the latest push.
| maxTaskPerSecond: params.WorkerActivitiesPerSecond, | ||
| taskPollers: []scalableTaskPoller{ | ||
| newScalableTaskPoller(poller, params.Logger, params.ActivityTaskPollerBehavior, params.serverSupportsAutoscaling), | ||
| newScalableTaskPoller(poller, params.Logger, params.ActivityTaskPollerBehavior, ""), |
There was a problem hiding this comment.
And this PollerTypeActivityTask
There was a problem hiding this comment.
Fixed in the latest push.
kritibehl
force-pushed
the
fix-task-poller-type-2235
branch
from
ec8976a to
f9bce07
Compare
yuandrew
left a comment
yuandrew
left a comment
There was a problem hiding this comment.
Thanks for the contribution! Left a few comments
| maxPollerCount: p.maximumNumberOfPollers, | ||
| minPollerCount: p.minimumNumberOfPollers, | ||
| logger: logger, | ||
| serverSupportsAutoscaling: serverSupportsAutoscaling, |
There was a problem hiding this comment.
Why did we stop passing serverSupportsAutoscaling here? We still reference it in newPollScalerReportHandle(). I think this means autoscaling pollers can't sale down until they receive their first server scaling decision.
There was a problem hiding this comment.
Restored — it's now threaded through the constructor and into newPollScalerReportHandle.
| localActivityWorker := newBaseWorker(baseWorkerOptions{ | ||
| slotSupplier: laParams.Tuner.GetLocalActivitySlotSupplier(), | ||
| maxTaskPerSecond: laParams.WorkerLocalActivitiesPerSecond, | ||
| slotSupplier: laParams.Tuner.GetLocalActivitySlotSupplier(), maxTaskPerSecond: laParams.WorkerLocalActivitiesPerSecond, |
There was a problem hiding this comment.
These 2 lines got mashed into a single line, please separate them back into 2 lines
There was a problem hiding this comment.
Fixed, split back onto separate lines.
| } | ||
| ) | ||
|
|
||
| func (s *ScalableTaskPollerSuite) TestNewScalableTaskPollerSetsTaskPollerType() { |
There was a problem hiding this comment.
Can we also include a test that verifies each poller type matches the poller (i.e. sticky workflow poller actually gets PollerTypeworkflowStickyTask) for each type?
There was a problem hiding this comment.
Added TestNewScalableTaskPollerAllTypes covering workflow, sticky, activity and nexus types.
kritibehl
force-pushed
the
fix-task-poller-type-2235
branch
from
f9bce07 to
48f0833
Compare
|
Friendly follow-up, I addressed the review comments and updated the branch, including the missing poller types, restored Happy to rebase or make any other adjustments if helpful. Thanks again for the review. |
yuandrew
left a comment
yuandrew
left a comment
There was a problem hiding this comment.
Sorry for the delay, was on vacation for a few weeks. Thanks for the contribution!
kritibehl
force-pushed
the
fix-task-poller-type-2235
branch
from
48f0833 to
0ef2a79
Compare
|
Thanks for the review and approval! I rebased onto latest main, resolved the conflicts, and reran go test ./... locally after the rebase: all passing. |
3 participants
What was changed
Updated
newScalableTaskPoller(...)to accept and setscalableTaskPoller.taskPollerTypeandserverSupportsAutoscaling.Wired poller types through when constructing scalable task pollers:
metrics.PollerTypeWorkflowTaskfor mixed and non-sticky workflow task pollersmetrics.PollerTypeWorkflowStickyTaskfor sticky workflow task pollersmetrics.PollerTypeActivityTaskfor activity task pollersmetrics.PollerTypeNexusTaskfor nexus task pollers""for local activity pollers (internal, not balanced)Also restored
serverSupportsAutoscalingthreading through the constructor intonewPollScalerReportHandle, fixed a formatting issue, and expanded test coverage with a table-driven test covering all poller types.Why?
pollerBalancerusesscalableTaskPoller.taskPollerTypeto register and balance poller types, but that field was never being set. This change wires the poller type through at construction time so balancing can operate on the intended poller types.Checklist
go test ./internal -run Pollergo test ./...