Add poller autoscaler#1956
Conversation
Quinn-With-Two-Ns
force-pushed
the
SDK-2478
branch
from
66ec6ab to
22ee23e
Compare
| // NewPollerBehaviorSimpleMaximum creates a PollerBehavior that allows the worker to start up to a maximum number of pollers. | ||
| // | ||
| // NOTE: Experimental | ||
| func NewPollerBehaviorSimpleMaximum( |
There was a problem hiding this comment.
Open question if these should take options instead
There was a problem hiding this comment.
Don't think so. Worst case scenario if we ever need to add anything it can be a new behavior.
| taskPollers: []scalableTaskPoller{ | ||
| newScalableTaskPoller(localActivityTaskPoller, params.Logger, NewPollerBehaviorSimpleMaximum(1)), | ||
| }, | ||
| taskProcessor: localActivityTaskPoller, |
There was a problem hiding this comment.
we're setting a "poller" as taskProcessor? That feels confusing
There was a problem hiding this comment.
The localActivityTaskPoller implements the poller interface and the task processing interface. I am not too inclined to split them , but do you have a better naming suggestion?
There was a problem hiding this comment.
hmm, not really 😅 it's probably less important for the Local Activity case
| numStickyPollerMetric *numPollerMetric | ||
| } | ||
|
|
||
| // workflowTaskProcessor implements processing of a workflow task and can create |
There was a problem hiding this comment.
I feel like the differentiation between Processor/Poller could be clearer. This comment says processor can create pollers, but newWorkflowTaskProcessor says it creates a new poller
| pollerCount: params.MaxConcurrentWorkflowTaskQueuePollers, | ||
| taskProcessor := newWorkflowTaskProcessor(taskHandler, contextManager, service, params) | ||
|
|
||
| var taskWorkers []scalableTaskPoller |
There was a problem hiding this comment.
There seem to be 3 terms that are all related but slightly different: task pollers, task workers, and task processor.
Looking at the code here, taskWorkers == taskPollers, and looking down in the file, taskProcessor: poller,. Do we need all 3 terms separately? Or can we combine taskWorkers and taskPollers, and differentiate them more clearly from taskProcessor?
There was a problem hiding this comment.
Yeah let me get rid of taskWorkers, it was a term I was using, but don't think it fits well
There was a problem hiding this comment.
So we have two interfaces task pollers and task processor that do two different things (poll tasks and process those tasks). For some task types the same struct implements both interfaces.
Sushisource
left a comment
Sushisource
left a comment
There was a problem hiding this comment.
Worth reviewing James' fix here https://github.com/temporalio/sdk-core/pull/941/files#diff-a65538899b187dda6f4128ecad6a38b908f9491bb63a4828f488e0e1d97f548fR121
To ensure that's not a problem here with the balancer either.
| // TODO(quinn): is there a way we can refactor this to avoid the function? | ||
| if func() bool { |
There was a problem hiding this comment.
You could set a var which is this fn's return value and continue if false, but it makes the defer stuff harder too... I think this is totally fine.
Quinn-With-Two-Ns
force-pushed
the
SDK-2478
branch
from
b78c6dc to
98b5605
Compare
| ) PollerBehavior { | ||
| initialNumberOfPollers := options.InitialNumberOfPollers | ||
| if initialNumberOfPollers <= 0 { | ||
| initialNumberOfPollers = defaultAutoscalingInitialNumberOfPollers // Default initial number of pollers. |
There was a problem hiding this comment.
Would it be helpful to log something here so users are aware their configuration is invalid?
There was a problem hiding this comment.
This is more to catch a non-set options then an invalid configuration. Setting to below zero is invalid and we could error here, but we don't for the old poller setting so I kept the same logic here.
|
|
||
| // balance checks if the poller type is balanced with other poller types. The goal is to ensure that | ||
| // at least one poller of each type is running before allowing any poller of the given type to increase. | ||
| func (pb *pollerBalancer) balance(ctx context.Context, pollerType string) error { |
There was a problem hiding this comment.
I think this could potentially be susceptible to some of the stuff fixed in temporalio/sdk-rust#941 -- but I'm not sure. Adding a test is probably worth it.
There was a problem hiding this comment.
Looking at the core issue, This code actually won't apply in the SimpleMaximum case. We didn't change any behaviour in the SimpleMaximum case for Go
Quinn-With-Two-Ns
force-pushed
the
SDK-2478
branch
from
70f5068 to
60aaf89
Compare
3 participants
Add poller autoscaler based on temporalio/sdk-rust@93471ac