-
Notifications
You must be signed in to change notification settings - Fork 801
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
Switch system worker to gRPC #4679
Switch system worker to gRPC #4679
Conversation
7f6a421
to
7b21152
Compare
7b21152
to
99842a6
Compare
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.
lgtm
common/rpc/outbounds.go
Outdated
@@ -81,6 +81,7 @@ func (b multiOutbounds) Build(grpc *grpc.Transport, tchannel *tchannel.Transport | |||
|
|||
type publicClientOutbound struct { | |||
address string | |||
isGrpc bool |
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.
nit isGRPC
|
||
builder, err = newPublicClientOutbound(makeConfig("localhost:1234", true, "invalid")) | ||
builder, err = newPublicClientOutbound(makeConfig("localhost:1234", "tchannel", true, "invalid")) | ||
require.EqualError(t, err, "create AuthProvider: invalid private key path invalid") | ||
|
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.
Nit: As you added require.True(t, builder.isGrpc)
, maybe add False check here?
require.False(t, builder.isGrpc) |
As per this PR: uber/cadence#4679
As per this PR: uber/cadence#4679
As per this PR: uber/cadence#4679
What changed?
Switch
PublicClient
used mainly by Worker role to communicate in gRPCAdditional
transport
config was introduced underpublicClient
to allow overriding transport back tchannel which otherwise would default to grpc.Why?
Final bits of gRPC migration on server side.
How did you test it?
Updated integration tests.
Locally started cadence canary. Issues batch terminate command which would start batch workflow on system worker.
Potential risks
Release notes
Documentation Changes