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.

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

Put the client connection behind a protocol #727

Merged
merged 2 commits into from
Feb 26, 2020

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Feb 25, 2020

Motivation:

Generated client stubs may only be initialized using a
ClientConnection which makes the assumption that a connection will
only ever be singular, and that all RPCs for that stub will run on a
single event loop.

However, there are scenarios where we may want to drive a stub and have
multiple connections (load balancing), or no real connection at all
(testing).

Modifications:

  • Generated client stubs now depend on the implementation of a GRPCChannel
  • The first argument label for the init in generated clients has
    changed from connection to client
  • Added fixits for the above
  • Updated generated code
  • GRPCChannels contract ensures that RPCs can be made but do not
    specify the transport.
  • GRPCClient now requires a GRPCChannel
  • GRPCClient provides higher level wrappers for the factory methods on
    GRPCChannel (i.e. provide defaults such as for call options)

Result:

  • We have a better ability, in the future, to use already-generated
    stubs with different GRPCChannels.
  • Cleaner abstraction between call and connection.

Motivation:

Generated client stubs may only be initialized using a
`ClientConnection` which makes the assumption that a connection will
only ever be singular, and that all RPCs for that stub will run on a
single event loop.

However, there are scenarios where we may want to drive a stub and have
multiple connections (load balancing), or no real connection at all
(testing).

Modifications:

- Generated client stubs now depend on the implementation of a `GRPCChannel`
- The first argument label for the `init` in generated clients has
  changed from `connection` to `client`
- Added fixits for the above
- Updated generated code
- `GRPCChannel`s contract ensures that RPCs can be made but do not
  specify the transport.
- `GRPCClient` now requires a `GRPCChannel`
- `GRPCClient` provides higher level wrappers for the factory methods on
  `GRPCChannel` (i.e. provide defaults such as for call options)

Result:

- We have a better ability, in the future, to use already-generated
  stubs with different `GRPCChannel`s.
- Cleaner abstraction between call and connection.
@glbrntt glbrntt requested a review from MrMage February 25, 2020 13:43
@glbrntt glbrntt added nio ⚠️ semver/major Breaks existing public API. labels Feb 25, 2020
Copy link
Collaborator

@MrMage MrMage left a comment

Choose a reason for hiding this comment

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

I didn't find much 😅 Seems like there is less happening here than I initially anticipated ;-)

@@ -25,32 +24,39 @@ import Logging
/// - `status`: the status of the gRPC call after it has ended,
/// - `trailingMetadata`: any metadata returned from the server alongside the `status`.
public final class ServerStreamingCall<RequestPayload: GRPCPayload, ResponsePayload: GRPCPayload>: BaseClientCall<RequestPayload, ResponsePayload> {
public init(
connection: ClientConnection,

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: extra newline

@@ -265,7 +296,7 @@ extension ClientConnection {
/// - Parameter backoffIterator: An `Iterator` for `ConnectionBackoff` providing a sequence of
/// connection timeouts and backoff to use when attempting to create a connection.
private class func makeChannel(
configuration: Configuration,
configuration: ClientConnection.Configuration,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the ClientConnection. prefix necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, will remove them, that was WIP that didn't get reverted.


extension GRPCClient {
@available(*, deprecated, renamed: "init(channel:defaultCallOptions:)", message: "You may also need to regenerate your client code")
public init(connection: ClientConnection, defaultCallOptions: CallOptions = CallOptions()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume we will delete these before 1.0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's right, this whole file will be removed (there's a TODO at the top of it)

@glbrntt
Copy link
Collaborator Author

glbrntt commented Feb 26, 2020

I didn't find much 😅 Seems like there is less happening here than I initially anticipated ;-)

Afraid so; nothing too exciting going on here!

@glbrntt glbrntt merged commit 50ccad2 into grpc:nio Feb 26, 2020
@glbrntt glbrntt deleted the gb-client-protocol branch February 26, 2020 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠️ semver/major Breaks existing public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants