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

Update google.golang.org/grpc and remove WithBalancerName which is deprecated #914

Merged
merged 4 commits into from
Apr 27, 2022

Conversation

cpanato
Copy link
Contributor

@cpanato cpanato commented Apr 27, 2022

  • update google.golang.org/grpc to v1.46.0
  • WithBalancerName is deprecated and removed, using the recommended way

Checklist

cpanato added 3 commits April 27, 2022 08:09
Signed-off-by: cpanato <ctadeu@gmail.com>
Signed-off-by: cpanato <ctadeu@gmail.com>
@@ -169,10 +169,10 @@ func main() {
}
res.InitialState(resolver.State{Addresses: addrs})
resolver.SetDefaultScheme(res.Scheme())
dialOpts = append(dialOpts, grpc.WithBalancerName(roundrobin.Name), grpc.WithResolvers(res))
dialOpts = append(dialOpts, grpc.WithDefaultServiceConfig(fmt.Sprintf(`{"loadBalancingConfig": [{"%s":{}}]}`, roundrobin.Name)), grpc.WithResolvers(res))
Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate the effort to keep some usage of the constants here, but given that this is a) difficult to read, and b) already contains a "magic string" for loadBalancingConfig key anyway, I'd be tempted to do what https://github.com/grpc/grpc-go/blob/master/examples/features/load_balancing/client/main.go#L77 does, here and below:

Suggested change
dialOpts = append(dialOpts, grpc.WithDefaultServiceConfig(fmt.Sprintf(`{"loadBalancingConfig": [{"%s":{}}]}`, roundrobin.Name)), grpc.WithResolvers(res))
dialOpts = append(dialOpts, grpc.WithDefaultServiceConfig(`{"loadBalancingConfig": [{"round_robin":{}}]}`), grpc.WithResolvers(res))

Is it just me, or is configuring options using json instead of a struct a bit weird? Anyway, thanks for doing this!

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that it's probably a better idea to inline this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks so much the feedback and review

@pav-kv
Copy link
Contributor

pav-kv commented Apr 27, 2022

@cpanato Could you please ensure to run go mod tidy, if not yet done?

Signed-off-by: cpanato <ctadeu@gmail.com>
@cpanato cpanato requested a review from mhutchinson April 27, 2022 14:22
@cpanato
Copy link
Contributor Author

cpanato commented Apr 27, 2022

@cpanato Could you please ensure to run go mod tidy, if not yet done?

i already did, re-run

$ go mod tidy

$ git status
On branch update-grpc
nothing to commit, working tree clean

@mhutchinson
Copy link
Contributor

/gcbrun

@mhutchinson mhutchinson merged commit cecb1d1 into google:master Apr 27, 2022
@cpanato cpanato deleted the update-grpc branch April 27, 2022 15:17
@cpanato
Copy link
Contributor Author

cpanato commented Apr 27, 2022

@mhutchinson any plans to have a release?

@mhutchinson
Copy link
Contributor

@cpanato I wouldn't say that I had it on my TODO list, but it's probably about time we cut new releases. @pavelkalinnikov is making some changes that cross-cut some of the repos at the moment. When that flurry of work calms down, I think that would be a good time to cut releases of the various repos (this one, trillian, and transparency-dev/merkle are the main ones being affected at the moment).

@mhutchinson
Copy link
Contributor

@cpanato hopefully you saw https://github.com/google/certificate-transparency-go/releases/tag/v1.1.3 :-)

@cpanato
Copy link
Contributor Author

cpanato commented May 27, 2022

thank you @mhutchinson

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