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

How about letting ClientBuilder expose io.grpc.Channel? #5922

Open
Lincong opened this issue Sep 23, 2024 · 5 comments
Open

How about letting ClientBuilder expose io.grpc.Channel? #5922

Lincong opened this issue Sep 23, 2024 · 5 comments

Comments

@Lincong
Copy link

Lincong commented Sep 23, 2024

Hi there, we build gRPC clients with Armeria 1.16.3. I wonder whether either of the following ideas could be good:

  1. Let com.linecorp.armeria.client.ClientBuilder class expose an API to build and return a io.grpc.Channel (implemented by ArmeriaChannel).
  2. Let ClientBuilder build a io.grpc.Channel once (at the first time when ClientBuilder#build(...) is called) and then re-use the channel for all future invocations of ClientBuilder#build(...).

Here is why I think these^ ideas would benefit our use case:

  1. Currently we provide an abstraction called GrpcChannel to our users. GrpcChannel is not aware of any stub type. Users can create stubs from GrpcChannel. For example, channel.createStub[StubType](...).
  2. Note that the above GrpcChannel is a custom abstraction defined by my team and it is NOT io.grpc.Channel.
  3. Each GrpcChannel instance contains its own ClientBuilder (decorators) and ClientFactory (connection pool). The goal is to have a clear separation of resources among GrpcChannel instances. Users can create and manage GrpcChannels by themselves. grpc-java provides a similar mental mode (example) to allow users to create stub(s) from a io.grpc.Channel (a grpc-java interface/API).
  4. In our current implementation, every channel.createStub[StubType](...) invocation requires calling ClientBuilder.build(stubClass) which creates a io.grpc.Channel. That means when N stubs are created from our GrpcChannel, there are N grpc-java io.grpc.Channels created (one for each stub).
  5. This^ creates a lot of overhead to our stub creation process. Ideally we hope to let every GrpcChannel instance have one io.grpc.Channel instance from which all stubs are built.
  6. We have done a POC for this^ idea and the performance improvement is significant. However, the POC is a bit hacky due to the fact that ClientBuilder does not provide an API to build io.grpc.Channel. Specifically, in the POC, after the first stub was created, we call stub.getChannel to get a reference to its underlying io.grpc.Channel built by ClientBuilder, then cache and reuse the io.grpc.Channel to build future stubs. If ClientBuilder provides an API to build and return a io.grpc.Channel (proposed idea 1), we can get rid of the hack. Or we can consider making ClientBuilder internally build a io.grpc.Channel once and keep reusing it for all invocations of ClientBuilder.build(stubClass). With this change, our code (in GrpcChannel.createStub) can simply call ClientBuilder.build(stubClass) to build every stub since ClientBuilder.build(stubClass) would be much more lightweight (because the io.grpc.Channel is reused).

Please let me know WYT. Thanks!

@minwoox
Copy link
Member

minwoox commented Sep 24, 2024

Thanks for the detailed information. 👍

Currently, we can't reuse an ArmeriaChannel for different stubs because the jsonMarshaller must be created individually for each stub:

serializationFormat, jsonMarshaller, simpleMethodNames);

We would need to disable JSON conversion to reuse the channel or introduce another implementation for the Channel. Given this limitation, I think a dedicated API for creating the channel is necessary, such as GrpcClientBuilder.buildChannel().

Do you have any idea on this? @line/dx

@Lincong
Copy link
Author

Lincong commented Sep 24, 2024

Currently, we can't reuse an ArmeriaChannel for different stubs because the jsonMarshaller must be created individually for each stub

I am curious why this is the case. Is it because every jsonMarshaller instance is associated with some stub-specific state?

I think a dedicated API for creating the channel is necessary, such as GrpcClientBuilder.buildChannel()

SG, it works for our use case. Thanks!

@minwoox
Copy link
Member

minwoox commented Sep 24, 2024

Is it because every jsonMarshaller instance is associated with some stub-specific state?

Yes, it is. A JSON marshaller has to know the type to serialize and deserialize.
Here are links:

@trustin
Copy link
Member

trustin commented Sep 25, 2024

Maybe we could refactor ArmeriaChannel so that it's capable of having different marshallers for different stub types? Could use weak-key map to avoid leak (if that's gonna be a problem)

@minwoox
Copy link
Member

minwoox commented Sep 25, 2024

Maybe we could refactor ArmeriaChannel so that it's capable of having different marshallers for different stub types?

Yeah, that's also a good idea. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants