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

Provide a solution for Netty version conflicts - grpc-netty requires specific Netty version #1370

Open
lhotari opened this issue Jun 12, 2024 · 19 comments

Comments

@lhotari
Copy link

lhotari commented Jun 12, 2024

Is your feature request related to a problem? Please describe.

grpc-netty is compatible only with specific Netty versions. For example, the officially supported version is 4.1.100.Final at the moment.

grpc provided grpc-netty-shaded package, but that's not compatible with jetcd. There are conflicts with io.grpc.netty.GrpcSslContexts/io.grpc.netty.shaded.io.grpc.netty.GrpcSslContexts and io.netty.handler.ssl.SslContext/io.grpc.netty.shaded.io.netty.handler.ssl.SslContext classes.

Describe the solution you'd like

I'm not sure what solution would be backwards compatible since GrpcSslContext and SslContext classes are exposed in the jetcd client builders.

Describe alternatives you've considered

Additional context

@lburgazzoli
Copy link
Collaborator

lburgazzoli commented Jun 12, 2024

Those classes have been exposed mainly because of my very limited time, but one of the original design goal of jetcd was to hide internal GRPC/Netty/Guava stuffs so I'd be happy if someone can help designing a replacement for the current ssl setup :)

@lhotari do you by chance have any time ?

@lhotari
Copy link
Author

lhotari commented Jun 12, 2024

@lhotari do you by chance have any time ?

@lburgazzoli I'd like to help, but unfortunately my time is very limited at the moment. I'm currently working 100%+ on Apache Pulsar and Apache Bookkeeper.

I was able to create a workaround for Apache Pulsar and Apache Bookkeeper for this issue. In Pulsar and Bookkeeper we use maven and I'm using maven-shade-plugin to transform the jetcd-core jar to use grpc-netty-shaded packages instead of grpc-netty. It also requires relocating vertx-grpc to use grpc-netty-shaded packages. This seems to be a sufficient workaround for Pulsar and Bookkeeper. The PR for Bookkeeper is apache/bookkeeper#4426.

For jetcd-core, it would be necessary to hide internals as you mentioned. After that has been completed, it would be possible to provide a shaded artifact that could either shade grpc completely or relocate vertx-grpc and the internal classes of jetcd-core to use grpc-netty-shaded instead of grpc-netty so that there wouldn't be similar Netty compatibility issues in the future.

@lburgazzoli
Copy link
Collaborator

ok, I may have some time next week so let see if I can make something not to horrible :) I'll keep you posted

@lhotari
Copy link
Author

lhotari commented Jun 14, 2024

I have reported the Netty 4.1.111.Final compatibility issue as grpc/grpc-java#11284 . There's a repro case using jetcd-core/jetcd-test 0.8.2, vert-grpc 4.5.8, grpc-java 1.64.0. It is described in the issue comment grpc/grpc-java#11284 (comment).

@lburgazzoli
Copy link
Collaborator

@lhotari do you have an example about how you configure ssl ?

@lhotari
Copy link
Author

lhotari commented Jun 17, 2024

@lburgazzoli
Copy link
Collaborator

So I'm thinking to add a new builder, similar to

/**
* Configure SSL/TLS context create through {@link GrpcSslContexts#forClient} to use.
*
* @param consumer the SslContextBuilder consumer
* @return this builder
* @throws SSLException if the SslContextBuilder fails
*/
public ClientBuilder sslContext(Consumer<SslContextBuilder> consumer) throws SSLException {
SslContextBuilder builder = GrpcSslContexts.forClient();
consumer.accept(builder);
return sslContext(builder.build());
}

but provided as an interface with an internal implementation to hide netty(grpc classes and to make the current setup deprecated for 1 release,

@lhotari would that work for you ?

@lhotari
Copy link
Author

lhotari commented Jun 17, 2024

but provided as an interface with an internal implementation to hide netty(grpc classes and to make the current setup deprecated for 1 release,

@lhotari would that work for you ?

I think so. I guess one of the implementation problems would be to support gprc-netty and grpc-netty-shaded in the same code base. Have you considered to removing the dependency to grpc-java completely? I learned from @vietj that vertx-grpc provides a new grpc-client that doesn't use grpc-java at all. Docs at https://vertx.io/docs/vertx-grpc/java/#_vert_x_grpc_client

@lburgazzoli
Copy link
Collaborator

Have you considered to removing the dependency to grpc-java completely? I learned from @vietj that vertx-grpc provides a new grpc-client that doesn't use grpc-java at all. Docs at https://vertx.io/docs/vertx-grpc/java/#_vert_x_grpc_client

oh, that's very interesting. I was not aware of that so I'll have a look because well, grpc-java is an immense source of troubles.

@lburgazzoli
Copy link
Collaborator

mh, the main issue with the vert.x client is that it seems it does not support any load balancing option and endpoint discovery, at least I've not been able to find it.

@lhotari @vietj is there any way to achieve it ?

@vietj
Copy link

vietj commented Jun 17, 2024

@lburgazzoli we have this coming in vertx 5 based on our own implementation actually (https://github.com/eclipse-vertx/vertx-service-resolver)

@lburgazzoli
Copy link
Collaborator

@lburgazzoli we have this coming in vertx 5 based on our own implementation actually (https://github.com/eclipse-vertx/vertx-service-resolver)

oh, that's great! do you by chance have an example of the integration with the vertx-grpc-client ?

@vietj
Copy link

vietj commented Jun 17, 2024

yes there is an integration since this is integrated with the vertx http client on which the grpc client is built.

here is how the HttpClient is created with a load balancer : https://github.com/eclipse-vertx/vert.x/blob/master/src/main/java/examples/HTTPExamples.java#L1405

the gRPC client wraps actually this client so we need in the gRPC client API to provide a similar buildr API

@vietj
Copy link

vietj commented Jun 17, 2024

which is this issue : eclipse-vertx/vertx-grpc#100

@vietj
Copy link

vietj commented Jun 17, 2024

Currently it can be achieved with https://github.com/eclipse-vertx/vertx-grpc/blob/main/vertx-grpc-client/src/main/java/io/vertx/grpc/client/GrpcClient.java#L95 which allows to wrap a client, we need to provide an integrated builder API instead

@lburgazzoli
Copy link
Collaborator

It seems switching to vert.x grpc client requires some more work than adding a builder, so I've opened a new issue #1372

@linghengqian
Copy link
Contributor

@lburgazzoli
Copy link
Collaborator

At this stage, the plan is to switch to vertx-grpc-client

@lburgazzoli
Copy link
Collaborator

@lhotari I've released version 0.8.3 with updated netty, the switch to vertx client would require some time since I'm running very low of spare time

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

No branches or pull requests

4 participants