Skip to content

Conversation

@peihe
Copy link
Contributor

@peihe peihe commented Jun 22, 2016

Spark dependency fix is included in this PR. This is to remove the beam-examples dependency from Spark.
failOnWarning is disabled for Flink for now.

The added dependencies in this PR come from "mvn dependency:analyze"

@peihe
Copy link
Contributor Author

peihe commented Jun 22, 2016

R: @amitsela for checking the correctness of the spark dependency.
R: @dhalperi for checking IO.
R: @mxm for flink
R: @davorbonaci for the overall PR

<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
<version>1.7.14</version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

version numbers should be inherited from root-level pom, I think. See grpc above, for instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@amitsela
Copy link
Member

Added comments, mainly relating to @dhalperi comment on inheriting versions for root level pom (and some suggestions for additional once for Spark's pom).
BTW, is there an additional benefit here ? besides clarity of course. I'm not familiar with analyze-only.

And how does this relate to removing beam-examples dependencies ? Why do we want to remove this dependency ? I would prefer a dependency to use beam-examples in the runner's tests on duplicating them.

@peihe
Copy link
Contributor Author

peihe commented Jun 23, 2016

Users should be able to run beam-examples with any runners, and beam-examples will need runtime dependency to runners-spark. I cannot add that runtime dependency when runners-spark depends on beam-examples.

Previously, runners-spark is pull dependency from other module, and runners-spark can be broken by dependency changes in other module (removing, changing version, and even adding). This is another benefits.

@amitsela
Copy link
Member

Makes sense. Thanks for clarifying that!
LGTM.

@@ -861,15 +861,6 @@
</plugin>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment should move.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@peihe peihe changed the title Enable dependency-plugin at global level. [BEAM-374] Enable dependency-plugin and failOnWarning at global level. Jun 24, 2016
@peihe
Copy link
Contributor Author

peihe commented Jun 24, 2016

@dhalperi PTAL, and merge if it looks good.

Thanks

@davorbonaci
Copy link
Member

LGTM.

@asfgit asfgit closed this in ab74eac Jun 24, 2016
@peihe peihe deleted the fix-spark-dep branch June 28, 2016 20:48
pl04351820 pushed a commit to pl04351820/beam that referenced this pull request Dec 20, 2023
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Amar3tto added a commit to akvelon/beam that referenced this pull request Mar 4, 2025
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.

5 participants