Skip to content

httpgrpc/server: Update NewClient to not use WithBalancerName#254

Merged
bboreham merged 1 commit intoweaveworks:masterfrom
jpkrohling:jpkrohling/issue239
Sep 13, 2022
Merged

httpgrpc/server: Update NewClient to not use WithBalancerName#254
bboreham merged 1 commit intoweaveworks:masterfrom
jpkrohling:jpkrohling/issue239

Conversation

@jpkrohling
Copy link
Copy Markdown
Contributor

@jpkrohling jpkrohling commented Aug 26, 2022

On projects consuming this module as a library, gRPC might be set at
newer versions where the deprecated #WithBalancerName has been removed
already. Given that the version used by this module already offers a
forward-compatible method, this commit uses that instead of the
deprecated #WithBalancerName.

This is a simplified version of #240, without touching the gRPC versions.

Fixes #239

Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de

@jpkrohling jpkrohling marked this pull request as draft August 26, 2022 21:33
@jpkrohling
Copy link
Copy Markdown
Contributor Author

For now, this is just an update of the PR opened by @metalmatze, I'll check the compat with previous versions and mark this PR as ready for review.

@jpkrohling jpkrohling force-pushed the jpkrohling/issue239 branch from fe73ea3 to 3082d6a Compare August 29, 2022 20:10
@jpkrohling
Copy link
Copy Markdown
Contributor Author

While I think the newer minor version of gRPC should be compatible with a previous minor in the same major, the change in the sum metrics for the bytes payload is suspicious. Instead of trying to figure out whether a compatibility problem does not exist (hard to prove), I decided just to simplify the original PR and just do the minimum necessary changes that work, namely, replacing the WithBalancerName with the new alternative, which was already available on the version used by this module.

@jpkrohling jpkrohling marked this pull request as ready for review August 29, 2022 20:13
Copy link
Copy Markdown
Collaborator

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Code change looks fine, but can you write a commit description that stands alone, fully describes this change, and doesn't require the reader to chase down three other things to understand it.

@bboreham
Copy link
Copy Markdown
Collaborator

bboreham commented Sep 1, 2022

go.mod was tidied up by #253; please rebase and explain any remaining changes.

On projects consuming this module as a library, gRPC might be set at
newer versions where the deprecated #WithBalancerName has been removed
already. Given that the version used by this module already offers a
forward-compatible method, this commit uses that instead of the
deprecated #WithBalancerName.

This is a simplified version of weaveworks#240, without touching the gRPC versions.

Fixes weaveworks#239

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
@jpkrohling
Copy link
Copy Markdown
Contributor Author

PR updated. The changes are now minimal.

@jpkrohling
Copy link
Copy Markdown
Contributor Author

I also just changed the GitHub PR description to match the commit message, as I'm not sure which merge strategy is used in this project.

@jpkrohling
Copy link
Copy Markdown
Contributor Author

Friendly ping: would this PR be accepted in its current form?

Copy link
Copy Markdown
Collaborator

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Thanks for the reminder, LGTM.

@bboreham bboreham merged commit e98fcdf into weaveworks:master Sep 13, 2022
MasslessParticle pushed a commit to grafana/loki that referenced this pull request Jan 12, 2023
…mmon` (#8125)

Not needed after weaveworks/common#254 and
weaveworks/common#262

More rationale on the issue.

Fixes #8085 

Signed-off-by: Kaviraj <kavirajkanagaraj@gmail.com>
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.

Broken grpc.WithBalancerName

2 participants