add poller autoscaling in activity and decision workers #1186
Conversation
Pull Request Test Coverage Report for Build 0182fefc-7a13-49a1-8887-001a9f9b0ce0
💛 - Coveralls |
shijiesheng
changed the title
|
|
||
| // optional: Sets the interval of poller autoscaling, between which poller autoscaler changes the poller count | ||
| // based on poll result. It takes effect if FeatureFlags.PollerAutoScalerEnabled is set to true. | ||
| // Default value is 1 min |
There was a problem hiding this comment.
I'm still experimenting on this. Since we ensure at least one poller is always working, the impact of this parameter should be low.
| targetMilliUsage uint64, | ||
| isDryRun bool, | ||
| cooldownTime time.Duration, | ||
| options pollerAutoScalerOptions, | ||
| logger *zap.Logger, | ||
| hooks ...func()) *pollerAutoScaler { | ||
| ctx, cancel := context.WithCancel(context.Background()) |
There was a problem hiding this comment.
is this a sign we should be passing in a context? e.g. to propagate shutdown information maybe.
|
|
||
| // Stop stops the poller autoscaler | ||
| func (p *pollerAutoScaler) Stop() { | ||
| p.cancel() |
There was a problem hiding this comment.
not relevant to this PR, but in general: we probably want a (time-bounded?) blocking "... and wait for shutdown to complete" behavior here.
otherwise it's impossible to rule out dangling goroutines (e.g. goleak tests) or race conditions (background loop triggering at the same time as cancelation).
internal/internal_worker_base.go
Outdated
| if bw.pollLimiter == nil || bw.pollLimiter.Wait(bw.limiterContext) == nil { | ||
| if bw.pollerAutoScaler != nil { | ||
| if pErr := bw.pollerAutoScaler.Acquire(1); pErr == nil { | ||
| defer bw.pollerAutoScaler.Release(1) | ||
| } else { | ||
| bw.logger.Warn("poller auto scaler acquire error", zap.Error(pErr)) | ||
| } | ||
| } |
There was a problem hiding this comment.
somewhat pedantic / realistic values seem unlikely to cause problems here, but I'm pretty sure this ordering means this is possible:
- autoscale from N to 1
- allow N to get through pollLimiter, while autoscaler allows one long request and blocks N-1
- long request finishes, N-1 run through as fast as possible, ignoring ratelimter
- sequentially, as autoscaler is at 1. or 2 at once if at 2, etc.
swapping the order seems better, since "waiting on the per-second request limiter" feels like "a concurrent poller" to me. the request itself isn't running, but we've reserved space for it to run "on time". so we'll degrade "concurrent requests on the network" (allowing it to go lower than specified, though it is in principle just a maximum) in favor of preserving "maximum requests per second".
There was a problem hiding this comment.
A minor thing worth changing, but otherwise this all looks reasonable / is mostly "just" a (good) refactor.
so 👍 structurally LGTM. I should read more of the autoscaler before reviewing in more depth though.
unrelated / not a problem for any realistic values for us, but the semaphore library doesn't detect overflows: https://github.com/marusama/semaphore/blob/master/semaphore.go#L98
we should probably make a PR for that. it'd be easy to fix.
* add pollerAutoScalerOptions * add tests for options * change autoscaler interface to include Stop method * add poller auto scaler to task worker * gracefully stop autoscaler * move up autoscaler in pollTask * add unit tests
What changed?
Why?
How did you test it?
Potential risks