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/10/31 14:28:27 UTC
[GitHub] flink pull request #4927: [FLINK-7778] [build] Shade ZooKeeper dependency
GitHub user zentol opened a pull request:
https://github.com/apache/flink/pull/4927
[FLINK-7778] [build] Shade ZooKeeper dependency
## What is the purpose of the change
This PR shades and relocates curator&zookeeper into flink-runtime to avoid dependency conflicts, primarily with hadoop.
This PR is based on work done by @StephanEwen. I only resolved the remaining issues:
* flink-mesos did not compile since it relies on the flink-runtime zookeeper dependency, which was now relocated (the process of which is not visible to flink-mesos). This was fixed by adding a relocation to flink-mesos for zookeeper that must at all times be in sync. A cleaner solution would be to create a dedicated flink-shaded-curator/zookeeper module that both flink-runtime/mesos work against.
* flink-tests test execution failed since it couldn't find a relocated curator test class. The relocation pattern for curator in flink-runtime was also applied to curator-test; however the relocated version is not included in the test jar and was thus not accessible to other modules. This was fixed by adding an relocation exclusion for classes from `org.apache.curator.test`.
* flink-yarn test execution failed since it couldn't find the unshaded guava Function class. Curator is indeed shading&relocating guava, unfortunately not all of it. As such it was needed to re-add flink-shaded-curator-recipes including the guava relocation. While it would have been possible to do the relocation in flink-runtime I instead opted for the known working solution.
## Brief change log
* shade&relocate zookeeper and flink-shaded-curator recipes into flink-runtime
* add a relocation for zookeeper to flink-mesos, in sync with flink-runtime
* remove flink-shaded-curator-test, since it is simply no longer needed
* add travis checks to ensure no unshaded curator/zk make it into flink-dist
* JobManagerHAJobGraphRecoveryITCase was moved to flink-runtime to resolve a ClassCastException related to ZK or smth
## Verifying this change
I've only made sure that things compile and tests can be run successfully. I have not tried this in an actual cluster setting due to time constraints.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (yes)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no)
- The serializers: (no)
- The runtime per-record code paths (performance sensitive): (no)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (yes)
## Documentation
- Does this pull request introduce a new feature? (no)
- If yes, how is the feature documented? (not applicable)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zentol/flink new_zkcur
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/4927.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 #4927
----
commit b69c21c36443355be95246b3fda795df4fb6524c
Author: Stephan Ewen <se...@apache.org>
Date: 2017-10-13T20:14:09Z
[FLINK-7778] [build] Shade ZooKeeper dependency
Shading the ZooKeeper dependency makes sure that this specific version of
ZooKeeper is used by the Flink runtime module. The ZooKeeper version is
sensitive, because we depend on bug fixes in later ZooKeeper versions
for Flink's high availability.
This prevents situations where for example a set of added dependencies (for
example transtive dependencies of Hadoop) cause a different ZooKeeper version
to be in the classpath and be loaded.
This commit also removes the 'flink-shaded-curator' module, which was originally
created to shade guava within curator, but is now obsolete, because newer
versions of curator shade guava already.
commit af6dc28cfcd8856ea812d8443c464bb1dedb4315
Author: zentol <ch...@apache.org>
Date: 2017-10-31T10:26:48Z
Resolve remaining issues
----
---
[GitHub] flink issue #4927: [FLINK-7778] [build] Shade Curator/ZooKeeper dependency
Posted by aljoscha <gi...@git.apache.org>.
Github user aljoscha commented on the issue:
https://github.com/apache/flink/pull/4927
Changes look good! @StephanEwen could you please have a look at the follow-up changes?
---
[GitHub] flink issue #4927: [FLINK-7778] [build] Shade Curator/ZooKeeper dependency
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4927
Very nice, thanks!
Merging this...
---
[GitHub] flink issue #4927: [FLINK-7778] [build] Shade Curator/ZooKeeper dependency
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4927
Looks good!
I would suggest two followups:
1. We can probably drop `flink-shaded-curator` as well, because curator shaded guava by itself in the currently used version (still needs an exclusion because they do not build the dependency reduced pom).
2. Add a `ZooKeeperAccess` util in `flink-runtime` for the access to ZooKeeper done in `flink-mesos`. That way, `flink-mesos` needs no direct ZK dependency.
---
[GitHub] flink issue #4927: [FLINK-7778] [build] Shade Curator/ZooKeeper dependency
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/4927
@zentol I like the changes!
Last two points:
1. Does it make sense to add only the non relocated guava classes in `flink-shaded-curator`? Meaning define a filter in the shading inclusion to only add `com.google.common.base.Function`, `com.google.common.base.Predicate`, and `com.google.common.reflect.TypeToken`?
2. We can probably remove the parent project `flink-shaded-curator` and make the `flink-shaded-curator-recipes` the main project.
---
[GitHub] flink issue #4927: [FLINK-7778] [build] Shade Curator/ZooKeeper dependency
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4927
I've added the `ZookeeperAccess` class as suggested.
---
[GitHub] flink issue #4927: [FLINK-7778] [build] Shade Curator/ZooKeeper dependency
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4927
@StephanEwen Implemented both suggestions.
---
[GitHub] flink pull request #4927: [FLINK-7778] [build] Shade Curator/ZooKeeper depen...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/4927
---
[GitHub] flink issue #4927: [FLINK-7778] [build] Shade Curator/ZooKeeper dependency
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4927
@StephanEwen
Regarding1) Curator 2.12 does not shade all of guava. This is the curation relocation definition for guava:
```
<relocation>
<pattern>com.google</pattern>
<shadedPattern>org.apache.curator.shaded.com.google</shadedPattern>
<excludes>
<exclude>com.google.common.base.Function</exclude>
<exclude>com.google.common.base.Predicate</exclude>
<exclude>com.google.common.reflect.TypeToken</exclude>
</excludes>
</relocation>
```
The excluded classes are _not_ unused, as the `Function` class is used by the `CuratorFramework`. So we still need to shade guava unfortunately.
As for 2), I'll take a look!
---
[GitHub] flink issue #4927: [FLINK-7778] [build] Shade Curator/ZooKeeper dependency
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4927
@StephanEwen I've added another commit that modifies flink-shaded-hadoop to include its own zookeeper dependency. So far we were re-using the runtime zk dependency.
---