Skip to content
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

Merged

Conversation

vytautas-karpavicius
Copy link
Contributor

@vytautas-karpavicius vytautas-karpavicius commented Dec 13, 2021

What changed?
Switch PublicClient used mainly by Worker role to communicate in gRPC

Additional transport config was introduced under publicClient 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

@vytautas-karpavicius vytautas-karpavicius changed the title Public client grpc Switch system worker to gRPC Dec 13, 2021
@coveralls
Copy link

coveralls commented Dec 13, 2021

Pull Request Test Coverage Report for Build 57b1aaa7-17aa-498b-a1df-9b34f23efe22

  • 42 of 57 (73.68%) changed or added relevant lines in 5 files are covered.
  • 51 unchanged lines in 10 files lost coverage.
  • Overall coverage increased (+0.003%) to 56.914%

Changes Missing Coverage Covered Lines Changed/Added Lines %
common/rpc/outbounds.go 15 17 88.24%
host/onebox.go 17 19 89.47%
cmd/server/cadence/server.go 0 11 0.0%
Files with Coverage Reduction New Missed Lines %
common/backoff/retry.go 1 93.91%
common/task/fifoTaskScheduler.go 2 85.57%
common/task/weightedRoundRobinTaskScheduler.go 2 89.64%
service/history/execution/mutable_state_builder.go 2 69.66%
service/history/queue/timer_queue_processor.go 2 58.8%
common/persistence/nosql/nosqlplugin/cassandra/workflow.go 3 55.49%
common/types/mapper/thrift/shared.go 4 63.22%
service/history/queue/timer_queue_processor_base.go 4 78.83%
common/persistence/nosql/nosqlplugin/cassandra/workflowUtils.go 12 77.73%
common/persistence/nosql/nosqlplugin/cassandra/workflowParsingUtils.go 19 86.84%
Totals Coverage Status
Change from base Build bb5e3efc-e0bc-4736-84cf-f05615f8c718: 0.003%
Covered Lines: 82798
Relevant Lines: 145479

💛 - Coveralls

@vytautas-karpavicius vytautas-karpavicius requested a review from a team December 13, 2021 12:12
@vytautas-karpavicius vytautas-karpavicius marked this pull request as ready for review December 13, 2021 12:12
Copy link
Contributor

@mantas-sidlauskas mantas-sidlauskas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@@ -81,6 +81,7 @@ func (b multiOutbounds) Build(grpc *grpc.Transport, tchannel *tchannel.Transport

type publicClientOutbound struct {
address string
isGrpc bool
Copy link
Contributor

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")

Copy link
Contributor

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?

Suggested change
require.False(t, builder.isGrpc)

@vytautas-karpavicius vytautas-karpavicius merged commit 3865361 into uber:master Dec 13, 2021
@vytautas-karpavicius vytautas-karpavicius deleted the public-client-grpc branch December 13, 2021 14:30
akijakya added a commit to banzaicloud/banzai-charts that referenced this pull request Nov 14, 2022
akijakya added a commit to banzaicloud/banzai-charts that referenced this pull request Nov 17, 2022
akijakya added a commit to banzaicloud/banzai-charts that referenced this pull request Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants