You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by twalthr <gi...@git.apache.org> on 2018/05/24 14:23:24 UTC
[GitHub] flink pull request #6073: [FLINK-9091] [table] Fix dependency convergence fo...
GitHub user twalthr opened a pull request:
https://github.com/apache/flink/pull/6073
[FLINK-9091] [table] Fix dependency convergence for flink-table
## What is the purpose of the change
Since FLINK-8511 solved most of the dependency convergence issues that were described in FLINK-9091. This PR solves the remaining ones by replacing the `dependencyManagement` section by exclusions.
## Brief change log
`flink-table/pom.xml` modified
## Verifying this change
Manually verified by e2e tests and SQL Client query execution.
## 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: no
- The S3 file system connector: no
## 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/twalthr/flink FLINK-9091
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/6073.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 #6073
----
commit 639df7cd3e690c2dcfaf35fb43d6b02ec48451f4
Author: Timo Walther <tw...@...>
Date: 2018-05-18T10:10:14Z
[FLINK-9091] [table] Fix dependency convergence for flink-table
----
---
[GitHub] flink issue #6073: [FLINK-9091] [table] Fix dependency convergence for flink...
Posted by pnowojski <gi...@git.apache.org>.
Github user pnowojski commented on the issue:
https://github.com/apache/flink/pull/6073
Ok, I see it now. This pull request's title is just a tad misleading (it doesn't fix all of the convergence for this module, just some selection of them)
---
[GitHub] flink issue #6073: [FLINK-9091] [table] Fix dependency convergence for flink...
Posted by pnowojski <gi...@git.apache.org>.
Github user pnowojski commented on the issue:
https://github.com/apache/flink/pull/6073
Does this fix or just improves dependency convergence? I'm asking because dependencies of `flink-table` (like `flink-runtime`) still have convergence errors.
---
[GitHub] flink issue #6073: [FLINK-9091] [table] Fix dependency convergence for flink...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the issue:
https://github.com/apache/flink/pull/6073
It only fixes the issues discussed in the corresponding JIRA. Dependency convergence issues with `flink-runtime` are a different topic.
---
[GitHub] flink pull request #6073: [FLINK-9091] [table] Fix dependency convergence fo...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/6073#discussion_r190855869
--- Diff: flink-libraries/flink-table/pom.xml ---
@@ -146,6 +147,12 @@ under the License.
<artifactId>flink-test-utils_${scala.binary.version}</artifactId>
<version>${project.version}</version>
<scope>test</scope>
+ <exclusions>
--- End diff --
Why does `flink-test-utils` even have a Guava dependency? Can we fix that?
---
[GitHub] flink issue #6073: [FLINK-9091] [table] Fix dependency convergence for flink...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/6073
I still don't know whether this is the way to go. The exclusion pattern can be icky when dealing with a single dependency A that pulls in 2 different versions of a dependency B. When excluding B you are excluding both versions, and have to add a separate dependency on B to **our** module. This not only obfuscates the dependency tree, but also directly blocks other efforts to forbid usages of unused dependencies.
---
[GitHub] flink issue #6073: [FLINK-9091] [table] Fix dependency convergence for flink...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the issue:
https://github.com/apache/flink/pull/6073
@zentol After FLINK-8511 the number of exclusions seem acceptable to me. What do you think?
---
[GitHub] flink issue #6073: [FLINK-9091] [table] Fix dependency convergence for flink...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the issue:
https://github.com/apache/flink/pull/6073
@pnowojski just pointed out that we can replace the Janino exclusion by a `dependencyManagement` entry in the root pom. I'm fine with this. But how should we deal with guava?
---
[GitHub] flink pull request #6073: [FLINK-9091] [table] Fix dependency convergence fo...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr closed the pull request at:
https://github.com/apache/flink/pull/6073
---
[GitHub] flink issue #6073: [FLINK-9091] [table] Fix dependency convergence for flink...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the issue:
https://github.com/apache/flink/pull/6073
We had a dependency on Janino before so this adds no additional dependency to our module. The only special case is about guava (as always). I think you have the bigger picture about Flink's dependency management, I'm open for other solutions (like disabling the enforcer plugin etc.).
---
[GitHub] flink pull request #6073: [FLINK-9091] [table] Fix dependency convergence fo...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6073#discussion_r191207284
--- Diff: flink-libraries/flink-table/pom.xml ---
@@ -146,6 +147,12 @@ under the License.
<artifactId>flink-test-utils_${scala.binary.version}</artifactId>
<version>${project.version}</version>
<scope>test</scope>
+ <exclusions>
--- End diff --
flink-test-utils depends on curator-test which depends on guava.
---
[GitHub] flink issue #6073: [FLINK-9091] [table] Fix dependency convergence for flink...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/6073
DependencyManagement in the root pom should cover dependencies that we share and expose across modules.
Enforcing convergence with one module (`flink-table`) for a dependency that is hidden (shaded) should be handled in that module, not clutter the project-global configuration.
---
[GitHub] flink issue #6073: [FLINK-9091] [table] Fix dependency convergence for flink...
Posted by pnowojski <gi...@git.apache.org>.
Github user pnowojski commented on the issue:
https://github.com/apache/flink/pull/6073
Converging dependencies by enforcing their versions in root's pom `dependencyManagement` section is the way that we have chosen previously, so it would be more consistent.
https://issues.apache.org/jira/browse/FLINK-7765
https://github.com/apache/flink/pull/4777
Thus I would put `janino`'s entries in root pom. Open question is what to do with `guava` since I do not know if this comment in root pom is still valid after 3 years and what was it all about:
```
<dependencyManagement>
<!-- WARN:
DO NOT put guava,
protobuf,
asm,
netty
here. It will overwrite Hadoop's guava dependency (even though we handle it
separatly in the flink-shaded-hadoop module).
-->
```
---