httpgrpc/server: Update NewClient to not use WithBalancerName #240
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
Additionally, gRPC was bumped to v1.34.0 to support the insecure.NewCredentials()
bboreham
reviewed
May 3, 2022
Comment on lines
+85
to
+87
| received_payload_bytes_bucket{method="gRPC",route="/grpc.health.v1.Health/Check",le="1.048576e+06"} 1 | ||
| received_payload_bytes_bucket{method="gRPC",route="/grpc.health.v1.Health/Check",le="2.62144e+06"} 1 | ||
| received_payload_bytes_bucket{method="gRPC",route="/grpc.health.v1.Health/Check",le="5.24288e+06"} 1 |
There was a problem hiding this comment.
Any idea why this changed? Seems like it lost a space in formatting.
| @@ -211,9 +215,8 @@ func TestGrpcStatsStreaming(t *testing.T) { | |||
| received_payload_bytes_bucket{method="gRPC",route="/middleware.EchoServer/Process",le="1.048576e+08"} 5 | |||
| received_payload_bytes_bucket{method="gRPC",route="/middleware.EchoServer/Process",le="2.62144e+08"} 5 | |||
| received_payload_bytes_bucket{method="gRPC",route="/middleware.EchoServer/Process",le="+Inf"} 5 | |||
| received_payload_bytes_sum{method="gRPC",route="/middleware.EchoServer/Process"} 1.5728664e+07 | |||
| received_payload_bytes_sum{method="gRPC",route="/middleware.EchoServer/Process"} 1.5728689e+07 | |||
There was a problem hiding this comment.
This says we received a different number of bytes, which makes me nervous about gRPC compatibility.
|
@metalmatze any updates when this PR could be merged? |
|
This week I'm at KubeCon and then need to see. If others could check in the meantime I'm happy getting it merged without me. |
|
For clarity, I am looking for someone to say "we tested forwards/backwards compatibility by doing xxx, and it was fine". |
|
Ping, in case any of you have updates on testing gRPC compatibility. |
|
Sorry, currently don't have the time to work on this. If someone does feel free to pick it up again. |
jpkrohling
added a commit
to jpkrohling/common
that referenced
this pull request
Aug 26, 2022
Fixes weaveworks#239, based on weaveworks#240. Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
jpkrohling
added a commit
to jpkrohling/common
that referenced
this pull request
Aug 29, 2022
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>
|
Hopefully #254 gets us there. Thanks @jpkrohling |
jpkrohling
added a commit
to jpkrohling/common
that referenced
this pull request
Sep 1, 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 weaveworks#240, without touching the gRPC versions. Fixes weaveworks#239 Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
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.
Additionally, gRPC was bumped to v1.34.0 to support the insecure.NewCredentials()
Closes #239
cc @bboreham