-
Notifications
You must be signed in to change notification settings - Fork 316
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
Comments
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 ? |
@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. |
ok, I may have some time next week so let see if I can make something not to horrible :) I'll keep you posted |
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). |
@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.) |
So I'm thinking to add a new builder, similar to jetcd/jetcd-core/src/main/java/io/etcd/jetcd/ClientBuilder.java Lines 290 to 302 in 4d93e3b
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 |
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 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 ? |
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 |
which is this issue : eclipse-vertx/vertx-grpc#100 |
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 |
It seems switching to vert.x grpc client requires some more work than adding a builder, so I've opened a new issue #1372 |
|
At this stage, the plan is to switch to vertx-grpc-client |
@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 |
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
andio.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
The text was updated successfully, but these errors were encountered: