Use direct outbound for history client #4619
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vytautas-karpavicius
force-pushed
the
history-direct-outbound
branch
2 times, most recently
from
3025999 to
9346303
Compare
vytautas-karpavicius
force-pushed
the
history-direct-outbound
branch
from
5367341 to
e630366
Compare
| func NewPeerResolver(numberOfShards int, membership membership.Monitor, addressMapper AddressMapperFn) PeerResolver { | ||
| return PeerResolver{ | ||
| numberOfShards: numberOfShards, | ||
| membership: membership, |
There was a problem hiding this comment.
If you need only history resolver, you can replace membership with resolver and pass history resolver here
There was a problem hiding this comment.
Agree, updated the PR.
mantas-sidlauskas
approved these changes
Nov 8, 2021
client/history/client.go
Outdated
| logger log.Logger, | ||
| ) Client { | ||
| return &clientImpl{ | ||
| numberOfShards: numberOfShards, | ||
| rpcMaxSizeInBytes: rpcMaxSizeInBytes, | ||
| tokenSerializer: common.NewJSONTaskTokenSerializer(), | ||
| timeout: timeout, | ||
| clients: clients, | ||
| client: client, | ||
| peer: peerResolver, |
There was a problem hiding this comment.
nit: rename peer to peerResolver to make it more obvious
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changed?
Client factory used to initialize and cache history clients for each peer. For each client a separate dispatcher was created via rpc factory. This is rather complicated setup and is not idiomatic on YARPC framework.
In order to route requests to different peers, yarpc provides direct peer chooser. It uses
ShardKeyset on the request to route this particular call to the given host. We do not need to initialize multiple dispatchers and multiple clients with caching on top.This PR:
history.PeerChooser, documents it and adds unit tests.history.Clientto drop client cache and instead addsWithShardKeyfor routing to appropriate instance.client.Factoryto wire up new setup.Why?
To move forward towards use of single yarpc dispatcher.
The similar effort is needed for matching client as well. After that we can basically drop rpc.Factory and use single Dispatcher everywhere.
How did you test it?
New unit tests.
Integration tests.
Locally with cadence canary.
Potential risks
This also brings one observable change. History client will no longer use caller name
history-service-client, but rather use the actual service name where the call is originating, for example:cadence-frontend.Even though the we can override callerName, at the moment I do not think this is needed. I'm not aware where this could be used other than yarpc metrics.
IMO, the actual name of caller service is more useful than a constant string.
Release notes
Documentation Changes