fix: Replace deprecated grpc.Dial with grpc.NewClient
#745
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
Propogates errors received from the rlp via gRPC when sending requests. Now that we don't block on dialing the server, we need to handle errors at the sending requests level. Signed-off-by: Andrew Crump <andrew.crump@broadcom.com>
Fixes the failing pool tests in `plumbing` and `rlp/internal/ingress` following the change to use `grpc.NewClient` instead of `grpc.Dial`. This is because they were testing that connections were initiated when a new doppler was added. However, with the change to NewClient, connections won't be made until the resulting grpc client connections are used for RPC. This also means that dopplers may be added to the pool map when in the past they would not be added due to the connection on Dial failing. One approach to getting the existing tests to pass would have been to call `Connect` on the grpc client connections to force them to leave idle mode. However, `Connect` is experimental, and really gRPC is encouraging us not to care about the connection state when creating client connections. To fix the tests we ended up just asserting on the pool size. One downside of this was that we couldn't see a nice way to assert that `Close` was called on the gRPC client connection when `Close` was called for a doppler address. We could have replaced the connection creation with an interface and mocked that but it didn't seem worth it. Signed-off-by: Carson Long <12767276+ctlong@users.noreply.github.com> Signed-off-by: Rebecca Roberts <rebecca.roberts@broadcom.com>
rroberts2222
approved these changes
Apr 17, 2024
|
@rroberts2222 and I tested these changes within CF-D and saw that they resulted in RLP and TrafficController not initiating any TCP connections to Dopplers until a client attempted to connect to them. This is in contrast to the old behaviour where the Doppler connection pools were pre-filled to the max. We think this change is fine. @acrmp and tested the RLP Gateway changes and saw that clients will consistently receive 503 errors now when the RLP is not available, e.g. from the log-stream plugin: The RLP Gateway logs will not emit stack traces in those cases, but will emit an error line for every failed request: |
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.
Description
Replaces
grpc.Dialusage withgrpc.NewClient. gRPC deprecated (and then briefly un-deprecated, but plan to re-deprecate)DialandDialContextin favor ofNewClient. See grpc/grpc-go#7010 for more info aboutNewClient.Type of change
Testing performed?
Checklist:
mainbranch, or relevant version branchIf you have any questions, or want to get attention for a PR or issue please reach out on the #logging-and-metrics channel in the cloudfoundry slack