Integrate shard distributor client in matching client #6622
Conversation
We now pass the shard distributor client from the resource where it's fully initialized instead of from the bean where the retry handler is still lacking.
jakobht
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
jakobht
force-pushed
the
integrate-shard-distributor-client-in-mathcing-client
branch
from
1188e18 to
0c055e0
Compare
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return c.client.RefreshTaskListPartitionConfig(ctx, request, append(opts, yarpc.WithShardKey(peer))...) | ||
| } | ||
|
|
||
| func (c *clientImpl) getShardOwner(ctx context.Context, taskListName string) (string, error) { |
There was a problem hiding this comment.
We have the peer resolver interface.
Could you consider implementing a new type of peer resolver and put the logic in the new implementation?
There was a problem hiding this comment.
+1. Maybe introduce a MultiPeerResolver object which satisfies PeerResolver interface. It can do take this responsibility (either calls legacy peerResolver or shardDistributor)
There was a problem hiding this comment.
Good points, will work on making this. Thanks!
taylanisikdemir
changed the title
| @@ -43,7 +43,7 @@ type ( | |||
| Bean interface { | |||
| GetHistoryClient() history.Client | |||
| GetHistoryPeers() history.PeerResolver | |||
| GetMatchingClient(domainIDToName DomainIDToNameFunc) (matching.Client, error) | |||
| GetMatchingClient(domainIDToName DomainIDToNameFunc, shardDistributorClient sharddistributor.Client) (matching.Client, error) | |||
There was a problem hiding this comment.
see the other comment. this interface change shouldn't be needed when sharddistributor.Client is an implementation detail of existing peer resolver.
| @@ -836,7 +836,7 @@ func (c *cadenceImpl) startMatching(hosts map[string][]membership.HostInfo, star | |||
|
|
|||
| // When there are multiple matching hosts the last client will overwrite previous ones. | |||
| // It should be fine because the underlying client bean logic should still pick the right destination. | |||
| matchingClient, err := clientBean.GetMatchingClient(matchingService.GetDomainCache().GetDomainName) | |||
| matchingClient, err := clientBean.GetMatchingClient(matchingService.GetDomainCache().GetDomainName, nil) | |||
There was a problem hiding this comment.
do we have any integration test which uses shard distributor? we should consider running existing set of integration tests with shard manager by making this configurable
What changed?
Matching client can now request the current owner of a tasklist from the shard distributor. This is guarded by a feature flag that defaults to the current sharding lookup.
Why?
We can now let clients lookup who the owner of a matching shard is in shard distributor. This is the first use of the spectator API in shard distributor
How did you test it?
Unit tests, and local runs
Potential risks
The new behavior is hidden behind the feature flag, so the risk is small.
Another risk is that this causes e.g. the matching client not to intitialize correctly, potentially causing crash loops. I have tested this locally to make sure it does not happen.
Release notes
Documentation Changes