-
Notifications
You must be signed in to change notification settings - Fork 132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add Concurrency entity for worker #1405
Changes from all commits
abf6b28
9010a8c
4ee4500
f065c10
a982c04
b99b8ce
dc99021
5bcc55b
f08bd7c
e6d7036
f453fb3
d2e13fd
0b11e7d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,6 +33,7 @@ import ( | |
"time" | ||
|
||
"go.uber.org/cadence/internal/common/debug" | ||
"go.uber.org/cadence/internal/worker" | ||
|
||
"github.com/uber-go/tally" | ||
"go.uber.org/zap" | ||
|
@@ -141,7 +142,7 @@ type ( | |
logger *zap.Logger | ||
metricsScope tally.Scope | ||
|
||
pollerRequestCh chan struct{} | ||
concurrency *worker.ConcurrencyLimit | ||
pollerAutoScaler *pollerAutoScaler | ||
taskQueueCh chan interface{} | ||
sessionTokenBucket *sessionTokenBucket | ||
|
@@ -167,11 +168,18 @@ func createPollRetryPolicy() backoff.RetryPolicy { | |
func newBaseWorker(options baseWorkerOptions, logger *zap.Logger, metricsScope tally.Scope, sessionTokenBucket *sessionTokenBucket) *baseWorker { | ||
ctx, cancel := context.WithCancel(context.Background()) | ||
|
||
concurrency := &worker.ConcurrencyLimit{ | ||
PollerPermit: worker.NewResizablePermit(options.pollerCount), | ||
TaskPermit: worker.NewChannelPermit(options.maxConcurrentTask), | ||
} | ||
Comment on lines
+171
to
+174
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently these are two quite different things, but they're sharing a "Permit" type (sharing the name is fine, their semantics are similar) All they have in common is Quota() int
Release() because one is change-able and one is not, one uses chans and one does not. Is there a need to combine their APIs in |
||
|
||
var pollerAS *pollerAutoScaler | ||
if pollerOptions := options.pollerAutoScaler; pollerOptions.Enabled { | ||
concurrency.PollerPermit = worker.NewResizablePermit(pollerOptions.InitCount) | ||
pollerAS = newPollerScaler( | ||
pollerOptions, | ||
logger, | ||
concurrency.PollerPermit, | ||
) | ||
} | ||
|
||
|
@@ -182,7 +190,7 @@ func newBaseWorker(options baseWorkerOptions, logger *zap.Logger, metricsScope t | |
retrier: backoff.NewConcurrentRetrier(pollOperationRetryPolicy), | ||
logger: logger.With(zapcore.Field{Key: tagWorkerType, Type: zapcore.StringType, String: options.workerType}), | ||
metricsScope: tagScope(metricsScope, tagWorkerType, options.workerType), | ||
pollerRequestCh: make(chan struct{}, options.maxConcurrentTask), | ||
concurrency: concurrency, | ||
pollerAutoScaler: pollerAS, | ||
taskQueueCh: make(chan interface{}), // no buffer, so poller only able to poll new task after previous is dispatched. | ||
limiterContext: ctx, | ||
|
@@ -244,11 +252,13 @@ func (bw *baseWorker) runPoller() { | |
select { | ||
case <-bw.shutdownCh: | ||
return | ||
case <-bw.pollerRequestCh: | ||
bw.metricsScope.Gauge(metrics.ConcurrentTaskQuota).Update(float64(cap(bw.pollerRequestCh))) | ||
// This metric is used to monitor how many poll requests have been allocated | ||
// and can be used to approximate number of concurrent task running (not pinpoint accurate) | ||
bw.metricsScope.Gauge(metrics.PollerRequestBufferUsage).Update(float64(cap(bw.pollerRequestCh) - len(bw.pollerRequestCh))) | ||
case <-bw.concurrency.TaskPermit.GetChan(): // don't poll unless there is a task permit | ||
// TODO move to a centralized place inside the worker | ||
// emit metrics on concurrent task permit quota and current task permit count | ||
// NOTE task permit doesn't mean there is a task running, it still needs to poll until it gets a task to process | ||
// thus the metrics is only an estimated value of how many tasks are running concurrently | ||
bw.metricsScope.Gauge(metrics.ConcurrentTaskQuota).Update(float64(bw.concurrency.TaskPermit.Quota())) | ||
bw.metricsScope.Gauge(metrics.PollerRequestBufferUsage).Update(float64(bw.concurrency.TaskPermit.Count())) | ||
if bw.sessionTokenBucket != nil { | ||
bw.sessionTokenBucket.waitForAvailableToken() | ||
} | ||
|
@@ -260,10 +270,6 @@ func (bw *baseWorker) runPoller() { | |
func (bw *baseWorker) runTaskDispatcher() { | ||
defer bw.shutdownWG.Done() | ||
|
||
for i := 0; i < bw.options.maxConcurrentTask; i++ { | ||
bw.pollerRequestCh <- struct{}{} | ||
} | ||
|
||
for { | ||
// wait for new task or shutdown | ||
select { | ||
|
@@ -294,10 +300,10 @@ func (bw *baseWorker) pollTask() { | |
var task interface{} | ||
|
||
if bw.pollerAutoScaler != nil { | ||
if pErr := bw.pollerAutoScaler.Acquire(1); pErr == nil { | ||
defer bw.pollerAutoScaler.Release(1) | ||
if pErr := bw.concurrency.PollerPermit.Acquire(bw.limiterContext); pErr == nil { | ||
defer bw.concurrency.PollerPermit.Release() | ||
} else { | ||
bw.logger.Warn("poller auto scaler acquire error", zap.Error(pErr)) | ||
bw.logger.Warn("poller permit acquire error", zap.Error(pErr)) | ||
} | ||
} | ||
|
||
|
@@ -333,7 +339,7 @@ func (bw *baseWorker) pollTask() { | |
case <-bw.shutdownCh: | ||
} | ||
} else { | ||
bw.pollerRequestCh <- struct{}{} // poll failed, trigger a new poll | ||
bw.concurrency.TaskPermit.Release() // poll failed, trigger a new poll by returning a task permit | ||
} | ||
} | ||
|
||
|
@@ -368,7 +374,7 @@ func (bw *baseWorker) processTask(task interface{}) { | |
} | ||
|
||
if isPolledTask { | ||
bw.pollerRequestCh <- struct{}{} | ||
bw.concurrency.TaskPermit.Release() // task processed, trigger a new poll by returning a task permit | ||
} | ||
}() | ||
err := bw.options.taskWorker.ProcessTask(task) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
// Copyright (c) 2017-2021 Uber Technologies Inc. | ||
// | ||
// Permission is hereby granted, free of charge, to any person obtaining a copy | ||
// of this software and associated documentation files (the "Software"), to deal | ||
// in the Software without restriction, including without limitation the rights | ||
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell | ||
// copies of the Software, and to permit persons to whom the Software is | ||
// furnished to do so, subject to the following conditions: | ||
// | ||
// The above copyright notice and this permission notice shall be included in | ||
// all copies or substantial portions of the Software. | ||
// | ||
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
// THE SOFTWARE. | ||
|
||
package worker | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
) | ||
|
||
type channelPermit struct { | ||
channel chan struct{} | ||
} | ||
|
||
// NewChannelPermit creates a static permit that's not resizable | ||
func NewChannelPermit(count int) ChannelPermit { | ||
channel := make(chan struct{}, count) | ||
for i := 0; i < count; i++ { | ||
channel <- struct{}{} | ||
} | ||
return &channelPermit{channel: channel} | ||
} | ||
|
||
func (p *channelPermit) Acquire(ctx context.Context) error { | ||
select { | ||
case <-ctx.Done(): | ||
return fmt.Errorf("failed to acquire permit before context is done") | ||
case p.channel <- struct{}{}: | ||
return nil | ||
} | ||
} | ||
Comment on lines
+41
to
+48
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pretty strong sign that there need to be more tests: without a |
||
|
||
// AcquireChan returns a permit ready channel | ||
func (p *channelPermit) GetChan() <-chan struct{} { | ||
return p.channel | ||
} | ||
|
||
func (p *channelPermit) Release() { | ||
p.channel <- struct{}{} | ||
} | ||
|
||
// Count returns the number of permits available | ||
func (p *channelPermit) Count() int { | ||
return len(p.channel) | ||
} | ||
|
||
func (p *channelPermit) Quota() int { | ||
return cap(p.channel) | ||
} | ||
|
||
// SetQuota on static permit doesn't take effect | ||
func (p *channelPermit) SetQuota(_ int) { | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
// Copyright (c) 2017-2021 Uber Technologies Inc. | ||
// | ||
// Permission is hereby granted, free of charge, to any person obtaining a copy | ||
// of this software and associated documentation files (the "Software"), to deal | ||
// in the Software without restriction, including without limitation the rights | ||
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell | ||
// copies of the Software, and to permit persons to whom the Software is | ||
// furnished to do so, subject to the following conditions: | ||
// | ||
// The above copyright notice and this permission notice shall be included in | ||
// all copies or substantial portions of the Software. | ||
// | ||
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
// THE SOFTWARE. | ||
|
||
package worker | ||
|
||
import ( | ||
"context" | ||
) | ||
|
||
var _ Permit = (*resizablePermit)(nil) | ||
var _ ChannelPermit = (*channelPermit)(nil) | ||
|
||
// ConcurrencyLimit contains synchronization primitives for dynamically controlling the concurrencies in workers | ||
type ConcurrencyLimit struct { | ||
PollerPermit Permit // controls concurrency of pollers | ||
TaskPermit ChannelPermit // controls concurrency of task processing | ||
} | ||
|
||
// Permit is an adaptive permit issuer to control concurrency | ||
type Permit interface { | ||
Acquire(context.Context) error | ||
Count() int | ||
Quota() int | ||
Release() | ||
SetQuota(int) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems worth splitting this out so you don't have to no-op it. It isn't even needed - this all compiles fine with |
||
} | ||
|
||
type ChannelPermit interface { | ||
Permit | ||
GetChan() <-chan struct{} // fetch the underlying channel | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these methods are unnecessary after
Permit
introductionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with this removed, this is basically a "periodically update a cache and broadcast" thing, rather than a resource-controller.
which seems fine, just checking that that's what is needed. without anything using the hooks or
GetCurrent()
I'm kinda struggling to figure out what's needed eventually vs something we should get rid of while we can.