httpgrpc/server: Update NewClient to not use WithBalancerName#240
httpgrpc/server: Update NewClient to not use WithBalancerName#240metalmatze wants to merge 2 commits intoweaveworks:masterfrom
Conversation
Additionally, gRPC was bumped to v1.34.0 to support the insecure.NewCredentials()
bboreham
left a comment
There was a problem hiding this comment.
I think the code changes are OK, but I'm a little nervous whether it is completely compatible.
Could you do a check with the two different versions as client and server and report back?
| 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.
| 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. |
Fixes weaveworks#239, based on weaveworks#240. Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
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 |
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>
Additionally, gRPC was bumped to v1.34.0 to support the insecure.NewCredentials()
Closes #239
cc @bboreham