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

Use the smaller split netty #508

Merged
merged 1 commit into from
Oct 30, 2019
Merged

Conversation

taer
Copy link
Contributor

@taer taer commented Oct 28, 2019

Reduce both size and conflicts with other netty projects

@mikkokar mikkokar merged commit b3f1946 into ExpediaGroup:master Oct 30, 2019
@taer taer deleted the splitNetty branch October 30, 2019 13:45
@dvlato
Copy link
Contributor

dvlato commented Oct 30, 2019

Sorry I couldn't review this before getting merged. I would say that this PR could cause actually conflicts: If a plugin tries to use netty dependencies that are not in Styx, we might end up with different netty modules (with different versions, so compiled against different versions of Netty) coexisting in our classpath. At least if we include netty-all, we would only have a version ...

@dvlato
Copy link
Contributor

dvlato commented Oct 30, 2019

@taer , what's your use case here?

@mikkokar
Copy link
Contributor

Hi @dvlato

We could declare a "supported" Netty version in styx BOM, and instruct the plugins to take it from there?

Also, as we discussed, plugins could shade their Netty out of sight.

Finally, we could move towards using styx as a library/framework instead of using it as a stand-alone styx with external plugins?

@taer
Copy link
Contributor Author

taer commented Oct 31, 2019

My usecase was effectively the same. I used asynchttpclient which depends on the netty pieces

[INFO] +- org.asynchttpclient:async-http-client:jar:2.4.9:compile
[INFO] |  +- org.asynchttpclient:async-http-client-netty-utils:jar:2.4.9:compile
[INFO] |  +- io.netty:netty-codec-socks:jar:4.1.35.Final:compile
[INFO] |  +- io.netty:netty-handler-proxy:jar:4.1.35.Final:compile
[INFO] |  +- io.netty:netty-transport-native-epoll:jar:linux-x86_64:4.1.35.Final:compile
[INFO] |  |  \- io.netty:netty-transport-native-unix-common:jar:4.1.35.Final:compile
[INFO] |  +- io.netty:netty-resolver-dns:jar:4.1.35.Final:compile
[INFO] |  |  \- io.netty:netty-codec-dns:jar:4.1.35.Final:compile

So I had to take care to force his version of io.netty:* to be the same as styx's io.netty:netty-all since to maven, they were both independent artifacts. I caught this when I had a
netty-codec-dns.jar and netty-all.jar in my resulting classpath at different versions.

@dvlato
Copy link
Contributor

dvlato commented Nov 1, 2019

So if I understood it correctly, the change does not really help with having to force the version the plugin uses, but just reduces the footprint of netty.

*With netty-all, instead of declaring the version of the dependencies we could have just excluded the transitive netty dependencies (coming from async-http-client) and the plugin would have used netty-all for all the netty related stuff. That's why I thought it was slightly better fit for dependency conflicts. But it doesn't make much of a difference.

Anyway, it's merged now! We could discuss at other point why we need async-http-client instead of using styx http client, and how we can improve the situation.

@taer
Copy link
Contributor Author

taer commented Nov 3, 2019

@dvlato in my experience, most dependencies use the small Netty. And the use of asyc http client actually isn't the issue.

The correct but possible painful one is to.... Embrace the Maven shade relocation.

Netty is a pretty common dependency with high performance java projects. So it makes sense that the Styx dependent version gets relocated and made internal. I know a recent styx update hid all the Netty specifics from the API, so it should be transparent. O

@mikkokar mikkokar mentioned this pull request Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants