-
Notifications
You must be signed in to change notification settings - Fork 435
Support for Channel specification on ServiceClient #202
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
Conversation
Adds support to allow consumers to specify a `Channel` when creating `ServiceClient` instances. This is something already supported by other gRPC language flavors (like [Python](https://github.com/grpc/grpc/blob/7542ba6e0caee740f124dc25706287fd62f546d8/examples/python/multiplex/multiplex_client.py#L110)), and alleviates the need to create a new channel every time a call is made. Usage: ```swift let sharedChannel = Channel(address: "https://api.myservice.com", secure: true) HelloWorldClient(channel: sharedChannel).getHelloWorld(...) OtherClient(channel: sharedChannel).getOther(...) ``` Resolves grpc#200
MrMage
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks, Michael! This is indeed sort-of covered by gRPCTests, but those tests are very generic and tests many things at once. So I would indeed appreciate a small dedicated test (in EchoTests or a new file ServiceClientTests, I'd say) that specifically tests using two ServiceClients with the same channel. Something like this:
let channel = Channel(...)
do {
let client = EchoClient(channel: channel)
XCTAssertEqual("foo", client.get(...).text)
}
// First client has left scope by now.
do {
let client = EchoClient(channel: channel)
XCTAssertEqual("foo", client.get(...).text)
}
Hope that makes sense.
|
|
||
| /// Create a client using a pre-defined channel. | ||
| public init(channel: Channel) { | ||
| gRPC.initialize() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove this gRPC.initialize() call, given that this is now done by Channel.init. Also in the other constructors above and below.
Also, I'd suggest moving this constructor below or above all the other ones?
|
LGTM. Thanks @rebello95 for adding this, and @MrMage for reviewing! I agree that future tests would be important for this, but will merge this now. |
Adds tests for support added in grpc#202 that allows `ServiceClient` instances to share a single `Channel`.
Adds tests for support added in grpc#202 that allows `ServiceClient` instances to share a single `Channel`.
Adds tests for support added in grpc/grpc-swift#202 that allows `ServiceClient` instances to share a single `Channel`.
Adds support to allow consumers to specify a
Channelwhen creatingServiceClientinstances.This is something already supported by other gRPC language flavors (like Python), and alleviates the need to create a new channel every time a call is made.
Usage:
Resolves #200