You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by mcfongtw <gi...@git.apache.org> on 2017/08/15 13:06:29 UTC
[GitHub] flink pull request #4545: [FLINK-6805] [Cassandra-Connector] Shade indirect ...
GitHub user mcfongtw opened a pull request:
https://github.com/apache/flink/pull/4545
[FLINK-6805] [Cassandra-Connector] Shade indirect netty4 dep in pom.xml
## Brief change log
- To relocate various *indirect* netty4 dep of ver 4.0.33.Final to classpath of Flink's flavor using Maven shade plugin.
## Verifying this change
- Passed local `mvn clean verify` as well as TravisCI tests
## What is the purpose of the change
- Shade *(indirect)* netty4 dependencies to avoid class clash in the future.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (yes / no) **slightly related**, shade a netty4 dep.
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (yes / no) no
- The serializers: (yes / no / don't know) no
- The runtime per-record code paths (performance sensitive): (yes / no / don't know) no
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (yes / no / don't know) no
## Documentation
- Does this pull request introduce a new feature? (yes / no) no
- If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented) N.A.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mcfongtw/flink FLINK-6805
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/4545.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 #4545
----
commit f852eccd24b44ceece6e74bc06c24a77a9acb151
Author: Michael Fong <mc...@gmail.com>
Date: 2017-08-15T06:22:23Z
[FLINK-6805] [Cassandra-Connector] Shade indirect netty4 dep
To relocate various *indirect* netty4 dep of ver 4.0.33.Final to
classpath of Flink flavor using Maven shade plugin.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4545: [FLINK-6805] [Cassandra-Connector] Shade indirect netty4 ...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4545
looks good to me, will merge this later.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4545: [FLINK-6805] [Cassandra-Connector] Shade indirect ...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4545#discussion_r133215094
--- Diff: flink-connectors/flink-connector-cassandra/pom.xml ---
@@ -39,6 +39,7 @@ under the License.
<properties>
<cassandra.version>2.2.5</cassandra.version>
<driver.version>3.0.0</driver.version>
+ <netty4.version>4.0.27.Final</netty4.version>
--- End diff --
is this used anywhere?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4545: [FLINK-6805] [Cassandra-Connector] Shade indirect ...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4545#discussion_r133218223
--- Diff: flink-connectors/flink-connector-cassandra/pom.xml ---
@@ -83,6 +85,10 @@ under the License.
<exclude>com.google.inject.**</exclude>
</excludes>
</relocation>
+ <relocation>
+ <pattern>io.netty</pattern>
+ <shadedPattern>org.apache.flink.shaded.netty4.io.netty</shadedPattern>
--- End diff --
you are relocating cassandra indirect netty dependency into the same namespace as Flink direct netty dependency, which will lead to dependency conflicts (because the sole way of distinguishing between class veresions, the package declaration, is identical).
The `shadedPattern` should be `org.apache.flink.cassandra.shaded.io.netty` instead.
* This is in line with the guava relocation (in the same file).
* the cassandra prefix introduces another layer of uniqueness which makes dependency conflicts less likely.
* I want to reserve the `org.apache.flink.shaded` namespace for dependencies from `flink-shaded`, to avoid accidental dependency clashes from being created (which would have happened had we merged the PR as is)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4545: [FLINK-6805] [Cassandra-Connector] Shade indirect ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/4545
---
[GitHub] flink pull request #4545: [FLINK-6805] [Cassandra-Connector] Shade indirect ...
Posted by mcfongtw <gi...@git.apache.org>.
Github user mcfongtw commented on a diff in the pull request:
https://github.com/apache/flink/pull/4545#discussion_r133399999
--- Diff: flink-connectors/flink-connector-cassandra/pom.xml ---
@@ -39,6 +39,7 @@ under the License.
<properties>
<cassandra.version>2.2.5</cassandra.version>
<driver.version>3.0.0</driver.version>
+ <netty4.version>4.0.27.Final</netty4.version>
--- End diff --
Good call! It's not used currently. I will remove it .
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4545: [FLINK-6805] [Cassandra-Connector] Shade indirect ...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4545#discussion_r133407236
--- Diff: flink-connectors/flink-connector-cassandra/pom.xml ---
@@ -83,6 +85,10 @@ under the License.
<exclude>com.google.inject.**</exclude>
</excludes>
</relocation>
+ <relocation>
+ <pattern>io.netty</pattern>
+ <shadedPattern>org.apache.flink.shaded.netty4.io.netty</shadedPattern>
--- End diff --
1) We haven't written them down anywhere; but there are examples on how the pattern should look like in many modules. (Granted, there are some relocations that don't follow the points I made; standardizing them is a work-in-progress) I would generally refer to existing relocations instead of some document that is bound to be out-dated at some point.
2) Currently we shade&relocate dependencies that already caused conflicts.
3) Yes, but there is no better solution. While it is possible to re-use some dependencies from Flink this tends to be a maintaining nightmare, as an upgrade to a dependency can suddenly cause issues in another part. The netty shading is a perfect example of this: we replaced the netty dependency and now the connector is broken.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4545: [FLINK-6805] [Cassandra-Connector] Shade indirect ...
Posted by mcfongtw <gi...@git.apache.org>.
Github user mcfongtw commented on a diff in the pull request:
https://github.com/apache/flink/pull/4545#discussion_r133404126
--- Diff: flink-connectors/flink-connector-cassandra/pom.xml ---
@@ -83,6 +85,10 @@ under the License.
<exclude>com.google.inject.**</exclude>
</excludes>
</relocation>
+ <relocation>
+ <pattern>io.netty</pattern>
+ <shadedPattern>org.apache.flink.shaded.netty4.io.netty</shadedPattern>
--- End diff --
Thanks for your review and reply! I see your points now, and I still have a few questions to the shading approach though:
1. There seems to be some policies for making shadedPattern for different scenarios. Would there be any external documents that possibly address these rules?
2. It is obvious to shade a *direct dependency, since we need it to be free of class clashes. How would we decide which *indirect dependencies to be shaded?
3. The benefit of shading is clear, but wouldn't it increase the jar file size if there are more dependencies that need to be managed later on - also the effort of managing the including/relocation rules.
I will update another version of PR later. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---