etcd-io / jetcd

etcd java client
Apache License 2.0
1.08k stars 314 forks source link

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

Open lhotari opened 3 weeks ago

lhotari commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

@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 https://github.com/apache/bookkeeper/pull/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 commented 3 weeks ago

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 commented 3 weeks ago

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 https://github.com/grpc/grpc-java/issues/11284#issuecomment-2168113850.

lburgazzoli commented 2 weeks ago

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

lhotari commented 2 weeks ago

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

@lburgazzoli This is the example from Apache Pulsar: https://github.com/apache/pulsar/blob/f3d4d5ac0442eed2b538b8587186cdc0b8df9987/pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/EtcdMetadataStore.java#L138-L148 .

(The Pulsar code is now using the custom shading solution that transforms jetcd-core to use gprc-netty-shaded.)

lburgazzoli commented 2 weeks ago

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

https://github.com/etcd-io/jetcd/blob/4d93e3b8fa8244cf17acff2b90c2a4f3b32ed3d2/jetcd-core/src/main/java/io/etcd/jetcd/ClientBuilder.java#L290-L302

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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

lburgazzoli commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

which is this issue : https://github.com/eclipse-vertx/vertx-grpc/issues/100

vietj commented 2 weeks ago

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 commented 2 weeks ago

It seems switching to vert.x grpc client requires some more work than adding a builder, so I've opened a new issue https://github.com/etcd-io/jetcd/issues/1372