matching: support for task list partitions #2229
Conversation
venkat1109
force-pushed
the
v_stl_6
branch
3 times, most recently
from
ae70cda
to
b1b4dcc
Compare
client/matching/lb.go
Outdated
| nPartitions dynamicconfig.IntPropertyFnWithTaskListInfoFilters, | ||
| ) string { | ||
|
|
||
| if forwardedFrom != "" || taskList.GetKind() == shared.TaskListKindSticky { |
There was a problem hiding this comment.
this assumes only one level of forwarding is possible currently, right?
There was a problem hiding this comment.
Nope, multiple levels of forwarding is ok. May be a comment might help here. When forwardedFrom is set, LB doesn't pick a partition because partition is already picked by the caller
client/matching/lb.go
Outdated
| defaultLoadBalancer struct { | ||
| nReadPartitions dynamicconfig.IntPropertyFnWithTaskListInfoFilters | ||
| nWritePartitions dynamicconfig.IntPropertyFnWithTaskListInfoFilters | ||
| domainIDToName func(string) (string, error) |
There was a problem hiding this comment.
domainIDToName DomainIDToNameFunc
There was a problem hiding this comment.
that would introduce a circular dependency. On the other hand, I can move the type definition of DomainIDToNameFunc() within this package. But that would be weird for the caller- who interacts only with client package
client/matching/lb.go
Outdated
| // NewLoadBalancer returns an instance of matching load balancer that | ||
| // can help distribute api calls across task list partitions | ||
| func NewLoadBalancer( | ||
| domainIDToName func(string) (string, error), |
There was a problem hiding this comment.
domainIDToName DomainIDToNameFunc
There was a problem hiding this comment.
see comment above
| case tm.queryTaskC <- task: | ||
| <-task.responseC | ||
| return nil, nil | ||
| case token := <-fwdrTokenC: |
There was a problem hiding this comment.
within this case should do another attempt to send task to tm.queryTaskC before forwarding?
| select { | ||
| case tm.taskC <- task: | ||
| return nil | ||
| case token := <-tm.fwdrAddReqTokenC(): |
There was a problem hiding this comment.
within this case should do another attempt to send task to tm.taskC before forwarding?
service/matching/matcher.go
Outdated
| @@ -148,12 +246,27 @@ func (tm *TaskMatcher) Poll(ctx context.Context) (*internalTask, error) { | |||
| case <-ctx.Done(): | |||
| tm.scope().IncCounter(metrics.PollTimeoutCounter) | |||
| return nil, ErrNoTasks | |||
| case token := <-tm.fwdrPollReqTokenC(): | |||
| if task, err := tm.fwdr.ForwardPoll(ctx); err == nil { | |||
There was a problem hiding this comment.
before actually polling from root / remote, maybe another attempt of polling from tm.taskC and tm.queryTaskC?
There was a problem hiding this comment.
basically, i think polling from local should have higher priority, event if task manager has token to poll from remote, same applies to add tasks
There was a problem hiding this comment.
I did consider this and then decided to drop this idea for the first round and here is the reasoning:
Before entering the select{} block which has both local task channels and forwarderTokenC, we always first exclusively attempt to match only with local task channels i.e. we always prioritize local pollers in the first select{} block before entering the second. In a busy task list, the first select{} should almost always match against a local poller. On the other hand, when its not matched, the task list probably isn't all that busy and its ok to forward. Having said that, my plan is to observe the forwardedFrom / matching metrics from a real stress test and use that to decide if adding this additional nested select{} is worth it. Till then, I prefer to start simple.
service/matching/matcher.go
Outdated
| @@ -162,19 +275,79 @@ func (tm *TaskMatcher) PollForQuery(ctx context.Context) (*internalTask, error) | |||
| case <-ctx.Done(): | |||
| tm.scope().IncCounter(metrics.PollTimeoutCounter) | |||
| return nil, ErrNoTasks | |||
| case token := <-tm.fwdrPollReqTokenC(): | |||
| if task, err := tm.fwdr.ForwardPoll(ctx); err == nil { | |||
This reverts commit bedd5e5.
Fixes #2098