Skip to content

Conversation

@zentol
Copy link
Contributor

@zentol zentol commented Nov 7, 2017

What is the purpose of the change

This PR replaces the dependency promotion from flink-runtime. The promotion appears to be interacting oddly with optional dependencies, and is generally prone to inducing unforeseen side-effects.

To accomplish the original goal behind the promotion I've added dependencies for akka-streams and akka-protobuf, which are the transitive dependencies that we want to keep being visible after the shading.

For reference, this is a comparison of the dependency footprint of flink-runtime as seen from another module (flink-dist), with and without dependency promotion:

Promotion enabled:
+- org.apache.flink:flink-runtime_2.11:jar:1.4-SNAPSHOT:compile
|  +- com.esotericsoftware.minlog:minlog:jar:1.2:compile
|  +- org.objenesis:objenesis:jar:2.1:compile
|  +- org.apache.flink:flink-queryable-state-client-java_2.11:jar:1.4-SNAPSHOT:compile
|  +- org.tukaani:xz:jar:1.0:compile
|  +- org.apache.avro:avro:jar:1.8.2:compile
|  +- org.codehaus.jackson:jackson-core-asl:jar:1.9.13:compile
|  +- org.codehaus.jackson:jackson-mapper-asl:jar:1.9.13:compile
|  +- com.thoughtworks.paranamer:paranamer:jar:2.7:compile
|  +- commons-codec:commons-codec:jar:1.10:compile
|  +- commons-logging:commons-logging:jar:1.1.3:compile
|  +- commons-lang:commons-lang:jar:2.6:compile
|  +- commons-configuration:commons-configuration:jar:1.7:compile
|  +- commons-digester:commons-digester:jar:1.8.1:compile
|  +- commons-beanutils:commons-beanutils-bean-collections:jar:1.8.3:compile
|  +- commons-io:commons-io:jar:2.4:compile
|  +- org.apache.flink:flink-shaded-netty:jar:4.0.27.Final-1.0:compile
|  +- org.apache.flink:flink-shaded-guava:jar:18.0-1.0:compile
|  +- org.apache.flink:flink-shaded-jackson:jar:2.7.9-2.0:compile
|  +- commons-cli:commons-cli:jar:1.3.1:compile
|  +- org.javassist:javassist:jar:3.18.2-GA:compile
|  +- com.typesafe.akka:akka-actor_2.11:jar:2.4.20:compile
|  +- com.typesafe:config:jar:1.3.0:compile
|  +- org.scala-lang.modules:scala-java8-compat_2.11:jar:0.7.0:compile
|  +- com.typesafe.akka:akka-stream_2.11:jar:2.4.20:compile
|  +- org.reactivestreams:reactive-streams:jar:1.0.0:compile
|  +- com.typesafe:ssl-config-core_2.11:jar:0.2.1:compile
|  +- org.scala-lang.modules:scala-parser-combinators_2.11:jar:1.0.4:compile
|  +- com.typesafe.akka:akka-protobuf_2.11:jar:2.4.20:compile
|  +- com.typesafe.akka:akka-slf4j_2.11:jar:2.4.20:compile
|  +- org.clapper:grizzled-slf4j_2.11:jar:1.0.2:compile
|  +- com.github.scopt:scopt_2.11:jar:3.5.0:compile
|  +- com.twitter:chill_2.11:jar:0.7.4:compile
|  \- com.twitter:chill-java:jar:0.7.4:compile
Promotion disabled (does NOT include additional akka dependencies):
+- org.apache.flink:flink-runtime_2.11:jar:1.4-SNAPSHOT:compile
|  +- org.apache.flink:flink-queryable-state-client-java_2.11:jar:1.4-SNAPSHOT:compile
|  +- commons-io:commons-io:jar:2.4:compile
|  +- org.apache.flink:flink-shaded-netty:jar:4.0.27.Final-1.0:compile
|  +- org.apache.flink:flink-shaded-guava:jar:18.0-1.0:compile
|  +- org.apache.flink:flink-shaded-jackson:jar:2.7.9-2.0:compile
|  +- commons-cli:commons-cli:jar:1.3.1:compile
|  +- org.javassist:javassist:jar:3.18.2-GA:compile
|  +- com.typesafe.akka:akka-actor_2.11:jar:2.4.20:compile
|  |  +- com.typesafe:config:jar:1.3.0:compile
|  |  \- org.scala-lang.modules:scala-java8-compat_2.11:jar:0.7.0:compile
|  +- com.typesafe.akka:akka-slf4j_2.11:jar:2.4.20:compile
|  +- org.clapper:grizzled-slf4j_2.11:jar:1.0.2:compile
|  +- com.github.scopt:scopt_2.11:jar:3.5.0:compile
|  \- com.twitter:chill_2.11:jar:0.7.4:compile
|     \- com.twitter:chill-java:jar:0.7.4:compile

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (yes)

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable)

@aljoscha
Copy link
Contributor

aljoscha commented Nov 7, 2017

Currently this fails because of missing entries in dependencyManagement in the root pom.

@zentol
Copy link
Contributor Author

zentol commented Nov 7, 2017

@aljoscha fixed

@aljoscha
Copy link
Contributor

aljoscha commented Nov 7, 2017

This looks good to go! I actually checked it out and manually ran the end-to-end tests because it's quicker than waiting for travis. 😅

@zentol
Copy link
Contributor Author

zentol commented Nov 8, 2017

merging.

asfgit pushed a commit that referenced this pull request Nov 8, 2017
@asfgit asfgit closed this in 1602bc3 Nov 8, 2017
@zentol zentol deleted the 8009b branch November 8, 2017 10:25
glaksh100 pushed a commit to lyft/flink that referenced this pull request Jun 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants