Enable flink dependency enforcement and make dependencies explicit #2549
Conversation
|
R: @aljoscha |
| <!-- | ||
| Force an upgrade on the version of Apache Commons from Flink to support DEFLATE compression. | ||
| --> | ||
| <dependency> | ||
| <groupId>org.apache.commons</groupId> | ||
| <artifactId>commons-compress</artifactId> | ||
| <version>[1.9,)</version> | ||
| <scope>runtime</scope> |
There was a problem hiding this comment.
I hesitated to put this in test, but not sure after reading the comment. Do you think that runtime still do the trick.
| @@ -46,7 +45,7 @@ public State getState() { | |||
| throws AggregatorRetrievalException { | |||
| throw new AggregatorRetrievalException( | |||
| "Accumulators can't be retrieved for detached Job executions.", | |||
| new NotImplementedException()); | |||
| new UnsupportedOperationException()); | |||
There was a problem hiding this comment.
I did this so we don't need the extra commons lang dependency for something so simple as this.
| @@ -38,7 +38,7 @@ | |||
| import org.apache.beam.sdk.util.WindowingStrategy; | |||
| import org.apache.beam.sdk.values.PCollectionView; | |||
| import org.apache.beam.sdk.values.TupleTag; | |||
| import org.apache.commons.lang.SerializationUtils; | |||
| import org.apache.commons.lang3.SerializationUtils; | |||
There was a problem hiding this comment.
And I moved this to use commons lang3 to align with other modules and beam-parent.
Notice that the scope is only test, so no issues.
|
|
||
| <build> | ||
| <plugins> | ||
| <plugin> |
There was a problem hiding this comment.
clean ups, this is all inherited now from the parent.
|
Run Flink ValidatesRunner |
|
Run Flink ValidatesRunner |
|
I created https://issues.apache.org/jira/browse/BEAM-1993 and https://issues.apache.org/jira/browse/BEAM-1994. Would you be interested in working on these? They simply remove code and restructure the modules. I'm asking because it would affect this PR. |
|
Sure I will take both if you don't mind after this one gets merged. |
runners/flink/runner/pom.xml
Outdated
|
|
||
| <dependency> | ||
| <groupId>org.apache.flink</groupId> | ||
| <artifactId>flink-annotations</artifactId> |
There was a problem hiding this comment.
I think this is only needed because EncodedValueTypeInformation.java has @PublicEvolving annotations, which it shouldn't have. If you remove those then you don't need this dependency.
|
Run Flink ValidatesRunner |
| @@ -18,7 +18,6 @@ | |||
| package org.apache.beam.runners.flink.translation.types; | |||
|
|
|||
| import org.apache.beam.sdk.coders.Coder; | |||
| import org.apache.flink.annotation.PublicEvolving; | |||
There was a problem hiding this comment.
This was removed to stop needing the flink-annotation dependency as suggested during the PR review..
|
Yep, this still LGTM! |
Be sure to do all of the following to help us incorporate your contribution
quickly and easily:
[BEAM-<Jira issue #>] Description of pull requestmvn clean verify. (Even better, enableTravis-CI on your fork and ensure the whole test matrix passes).
<Jira issue #>in the title with the actual Jira issuenumber, if there is one.
Individual Contributor License Agreement.