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.

Sign up for GitHub

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

Jump to bottom

fix: Replace deprecated grpc.Dial with grpc.NewClient #745

Merged
merged 3 commits into from
Apr 17, 2024
Merged

fix: Replace deprecated grpc.Dial with grpc.NewClient #745

merged 3 commits into from
Apr 17, 2024

Commits on Apr 17, 2024

  1. Replace usages of grpc.Dial with grpc.NewClient

    @acrmp @ctlong
    acrmp authored and ctlong committed Apr 17, 2024
    Configuration menu
    Copy the full SHA
    368f6ba View commit details
    Browse the repository at this point in the history

  2. Propagate errors in rlp log client streaming 

    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>
    @ctlong @acrmp
    ctlong authored and acrmp committed Apr 17, 2024
    Configuration menu
    Copy the full SHA
    efe712a View commit details
    Browse the repository at this point in the history

  3. chore: Make pool tests pass 

    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>
    @acrmp @ctlong
    acrmp authored and ctlong committed Apr 17, 2024
    Configuration menu
    Copy the full SHA
    3f82e26 View commit details
    Browse the repository at this point in the history