You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by zentol <gi...@git.apache.org> on 2017/12/04 13:08:13 UTC
[GitHub] flink pull request #5118: [FLINK-8193][quickstart] Cleanup quickstart poms
GitHub user zentol opened a pull request:
https://github.com/apache/flink/pull/5118
[FLINK-8193][quickstart] Cleanup quickstart poms
## What is the purpose of the change
This PR addresses 2 problems in the quick start poms:
1) The poms declared unused dependencies (flink-clients), and were missing used dependencies (flink-core, scala-library).
2) The manual dependency exclusion using the maven-shade-plugin was prone to being out-dated.
This PR fixes the missing/unused dependencies, and removes the manual exclusion so there is now only one correct way of building the jar, which is using the `build-jar` profile.
## Verifying this change
This change was verified locally. I installed the archetypes locally and created new projects from them.
For verification I executed the programs in the IDE and packaged the jar with the `build-jar` profile and checked whether that they don't contain any dependencies.
## Documentation
- Does this pull request introduce a new feature? (no)
- If yes, how is the feature documented? (docs)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zentol/flink 8193
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/5118.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #5118
----
commit 0f9697fdb4efb129458139d2fafbf20cc1faaa23
Author: zentol <ch...@apache.org>
Date: 2017-12-04T12:38:17Z
add missing / remove unused dependencies
commit f87b140083d253ffcb453f97aabf90fbfcb59098
Author: zentol <ch...@apache.org>
Date: 2017-12-04T12:38:33Z
Remove manual shading exclusion rules
----
---
[GitHub] flink issue #5118: [FLINK-8193][quickstart] Cleanup quickstart poms
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/5118
I am unsure here. The warning is nice, but without some exclusions from the shading phase, we will have many users wich build gigantic jars (happens to me as well sometimes). Duplicating dependencies into the user jar is more dangerous now that it used to be, given that we have the inverted classloading on as a default. We saw that with Avro, and probably have similar issues with Kryo and custom serializers as well, if they end up both in app classpath and in the user jar.
I know its harder to maintain, but in this case I think that immediate user experience trumps maintenance effort, so I would vote to add the exclusions back. The exclusions set should be much smaller now that we don't have Hadoop as a dependency here anymore. Having at least the flink projects (and flink-shaded projects) as exclusions, as well as scala / akka / kryo should really help.
We can use wildcard exclusions to make it more future safe, like replacing scala version with `*` (`<exclude>org.apache.flink:flink-runtime*</exclude>`) and excluding all artifacts for a group, like `<exclude>org.scala-lang:*</exclude>` and `<exclude>akka:*</exclude>`.
---
[GitHub] flink issue #5118: [FLINK-8193][quickstart] Cleanup quickstart poms
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/5118
Note that the current approach also prints a warning if `mvn verify` is executed.
---
[GitHub] flink issue #5118: [FLINK-8193][quickstart] Cleanup quickstart poms
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/5118
+1
---
[GitHub] flink issue #5118: [FLINK-8193][quickstart] Cleanup quickstart poms
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/5118
+1 for the latest suggestion
---
[GitHub] flink pull request #5118: [FLINK-8193][quickstart] Cleanup quickstart poms
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/5118
---
[GitHub] flink issue #5118: [FLINK-8193][quickstart] Cleanup quickstart poms
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/5118
merging.
---
[GitHub] flink issue #5118: [FLINK-8193][quickstart] Cleanup quickstart poms
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/5118
`flink-core` is a direct dependency due to the usage of the [Collector](https://github.com/apache/flink/blob/master/flink-quickstart/flink-quickstart-java/src/main/resources/archetype-resources/src/main/java/WordCount.java) class which is part of flink-core.
---
[GitHub] flink issue #5118: [FLINK-8193][quickstart] Cleanup quickstart poms
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/5118
Is there a way to fail the build if users do not use the `-Pbuild-jar` profile? I would assume that right now, at least half of the users build a mother-of-all-jars that includes all of Flink.
---
[GitHub] flink issue #5118: [FLINK-8193][quickstart] Cleanup quickstart poms
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/5118
That should work.
---
[GitHub] flink issue #5118: [FLINK-8193][quickstart] Cleanup quickstart poms
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/5118
Is it possible to print that only in the `package` phase? (or whatever it's called)
---
[GitHub] flink issue #5118: [FLINK-8193][quickstart] Cleanup quickstart poms
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/5118
I'll add flink-clients again with a comment.
---
[GitHub] flink issue #5118: [FLINK-8193][quickstart] Cleanup quickstart poms
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/5118
I like it this way, but what do you think, @StephanEwen? Is that a strong preference for "fail the build"?
---
[GitHub] flink issue #5118: [FLINK-8193][quickstart] Cleanup quickstart poms
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/5118
As for failing the build, I don't like that option, even if I find a way to do it. We _do not know_ how such a kill-switch would interact with various IDEs and tools.
What we could do however is print a warning if mvn package/install is invoked without specifying the build-jar profile.
---
[GitHub] flink issue #5118: [FLINK-8193][quickstart] Cleanup quickstart poms
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/5118
Updated the PR.
---
[GitHub] flink issue #5118: [FLINK-8193][quickstart] Cleanup quickstart poms
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/5118
Can't we do what I suggested earlier: don't build a fat jar if you don't specify the `build-jar` profile. That way you get a very slim jar that has no dependencies.
---
[GitHub] flink issue #5118: [FLINK-8193][quickstart] Cleanup quickstart poms
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/5118
Looks good.
Could you double check what gets packaged into the fat jar? In the previous version, the `slf4j-api` and `jsr305` dependencies ended up in the fat jar, even though they should be provided.
---
[GitHub] flink issue #5118: [FLINK-8193][quickstart] Cleanup quickstart poms
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/5118
The comment about "adding to the exclusions" under `a)` is outdated now. Should we maybe add the shade plugin in the profile, so that users don't accidentally build a super-fat jar?
Overall, I like the simplification.
---
[GitHub] flink issue #5118: [FLINK-8193][quickstart] Cleanup quickstart poms
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/5118
After adding kafka09/cassandra, `slf4j-api` and `force-shading` where included in the jar.
I've added exclusions for these dependencies to the shade-plugin configuration, as well as for jsr305 just to be safe.
---
[GitHub] flink issue #5118: [FLINK-8193][quickstart] Cleanup quickstart poms
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/5118
I think that the change of `1.` is actually not necessary. The dependencies were good in my opinion:
- `flink-core` is not directly needed, because it is a transitive dependency of `flink-java` and `flink-streaming-java`. Omitting this unnecessary dependency makes it easier to keep the `-Pbuild-jar` profile in sync with the normal profile.
- We added `flink-clients` explicitly because that is actually really the dependency needed to execute programs (is needed in the DataSet API, flink-java is not enough). It is only redundant at the moment because `flink-streaming-java` depends on `flink-clients`, which is not really nice and probably subject to change in the future.
---
[GitHub] flink issue #5118: [FLINK-8193][quickstart] Cleanup quickstart poms
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/5118
that's what we're already doing (well technicall yin the `pre-package` phase).
Thing is that the `package` phase is _always_ executed before `verify`, and we can't get around that aside from building a custom life-cycle. And we have to print a warning in this case, as the jar is still created and thus could be used.
---