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

ConnectionState.connect() should use generic Grpc.newChannelBuilderForAddress() #249

Open
coreydowning opened this issue Nov 2, 2023 · 6 comments · May be fixed by #251
Open

ConnectionState.connect() should use generic Grpc.newChannelBuilderForAddress() #249

coreydowning opened this issue Nov 2, 2023 · 6 comments · May be fixed by #251
Assignees
Labels
enhancement New feature or request

Comments

@coreydowning
Copy link

EventStoreDB-Client Version: 5.1.0

The ConnectionState.connect(...) method currently creates a managed gRPC channel using the NettyChannelBuilder from io.grpc:grpc-netty-shaded. We have a project that uses gRPC for multiple things and needs to use the io.grpc:grpc-netty transport instead of the shaded one. Because the connect method uses the netty shaded channel builder, this is impossible.

My suggested change is to move to using one of the flavors of Grpc.newChannelBuilder() function, allowing gRPC to dynamically choose the correct transport based on what is available on the classpath.

@YoEight YoEight added the enhancement New feature or request label Nov 2, 2023
@YoEight
Copy link
Member

YoEight commented Nov 2, 2023

Hey @coreydowning, I wasn't aware of that. I'll have that patch sorted out as soon as possible

@YoEight YoEight self-assigned this Nov 2, 2023
@coreydowning
Copy link
Author

Thank you very much! I sorted out most of it but haven't figured out an alternative to this

if (!settings.isTlsVerifyCert())
                    sslContextBuilder.trustManager(InsecureTrustManagerFactory.INSTANCE);

@YoEight
Copy link
Member

YoEight commented Nov 3, 2023

So far it seems impossible to keep feature parity while being grpc-backend agnostic. I can replicate everything but being able to disable certificate verification, which is quite useful in testing/dev environments. I'm open to suggestion, the related PR is linked to this issue.

@coreydowning
Copy link
Author

I will take a look at this today and see if I can come up with anything.

@michael-schnell
Copy link

michael-schnell commented Dec 31, 2023

Good idea to decouple the stuff! The GRPC client currently does not work with Quarkus native (GraalVM) because of multiple problems caused by the grpc-netty-shaded implementation. It would be great if it would be possible to use the Quarkus GRPC client instead.

@michael-schnell
Copy link

Any update?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants