-
Notifications
You must be signed in to change notification settings - Fork 13.8k
[FLINK-7840] [build] Shade netty in akka #4827
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
Conversation
| <relocations combine.children="append"> | ||
| <relocation> | ||
| <pattern>org.jboss.netty</pattern> | ||
| <shadedPattern>org.apache.flink.shaded.testutils.org.jboss.netty</shadedPattern> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain more why we need to shade this here? are we hiding some transitive netty dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its not a transitive dependency, but a direct one. Test-utils depends on netty directly for the network connection proxy. Because Netty clashes often and such clashes have made tests unstable in a very subtle way, I wanted to hide netty here again to avoid such future problems
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok. How come we aren't using flink-shaded-netty for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question, maybe @pnowojski can answer that... My guess is he was more familiar with the Netty 3 API (org.jboss.netty) and our shaded netty is Netty 4 (io.netty).
| </goals> | ||
| <configuration> | ||
| <!-- we need this to avoid having to specify all of akka's dependencies --> | ||
| <promoteTransitiveDependencies>true</promoteTransitiveDependencies> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why exactly do we need this? AFAIK you can shade transitive dependencies without promoting them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds akka-remote's dependencies as direct dependencies to flink-runtime (like for example reactive-streams).
Without promoting transitive dependencies, we would need to add all transitive dependencies of akka-remote to the shaded jar, because akka-remote is removed as a dependency which also removes its transitive dependencies.
We could do that, but since we cannot relocate many of them (Scala) I though to keep the set of dependencies that are "present but undeclared" small and treat akka-remote's dependencies like regular dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also add all transitive dependencies of akka-remote to the shaded jar. Its a similar consideration as "add all akka, or only akka-remote".
|
+1 |
|
Thanks for the review, merging... |
|
Merging as soon as Travis gives a green light on the rebased branch... |
|
Merged in 8595dad |
|
Is there any way to shade all akka dependencies ? In my project, scala has been shaded in another jar, which cause the failure of flink's initilization. |
What is the purpose of the change
This change shade's Akka's dependency on Netty away.
Note: Akka itself cannot be shaded away, there is some magic in Scala and annotations that Scala adds to preserve some compile time information, which make shading impossible (results in inconsistent code). That creates some problems.
I tried several approaches to shading Akka's Netty:
Add all Akka dependencies to
flink-runtime. The dependencies disappear due to shading, but the classes are present in the same namespace.This leads to problems. For example, tests pull in the regular akka dependencies as well (transitively from
akka-testkit) and for some reason it creates problems with the mixing of classes loaded fromflink-runtimeandakka-actor. One could fix that by adding all other akka dependencies asprovidedwhereverakka-testkitis used.Similarly, users that want to use akka have to add their own dependency to akka and will get a similar clash of classes.
This can be summed up as: Adding a dependency to a shaded jar (hence removing the dependency) without relocating the classes creates problems with dependency management.
Add only
akka-remoteto theflink-runtimejar. That is the minimum we need to add to shade the Netty dependency. It softens the problem mentioned in (1), because only one of the akka dependencies is present in theflink-runtimejar, but it does not solve it completely.We shade all akka dependencies into the
flink-distjar anyways, so anyone running a job with Flink will need to set all its akka dependencies toprovidedand assume Flink's version anyways (reloading in a different classloader through child-first loading as a workaround aside).So from the user's perspective, akka is always provided. Child-first class loading can save the day for some users (allowing them to have a different akka version in the user code class loader than Flink has in the system class loader), but we should not strictly rely on that - its a pretty delicate thing.
Brief change log
flink-runtimeto provided.akka-remote_*,io.nettyandorg.uncommons.mathto the shadedflink-runtimejarorg.jboss.nettyandorg.uncommons.mathflink-testutilsfor the network proxy and shade it there as well.Verifying this change
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (yes / no)Documentation