[FLINK-15868][kinesis] Resolve version conflict between jackson-core and jackson-dataformat-cbor #11006

tweise · 2020-02-03T23:19:48Z

What is the purpose of the change

For the Kinesis consumer to work, jackson-core and jackson-dataformat-cbor need to be at the same version. This change will ensure that users get the same version w/o having to override the jackson-dataformat-cbor dependency.

Note that the versions are consistent in https://mvnrepository.com/artifact/com.amazonaws/aws-java-sdk-core/1.11.603 - the problem was introduced by the dependency management in Flink.

Verifying this change

Run mvn dependency:tree on a downstream project and check that versions are same. Was also tested with the 1.10 RC1 with one of our internal deployments.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
The serializers: (yes / no / don't know)
The runtime per-record code paths (performance sensitive): (yes / no / don't know)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (yes / no / don't know)
The S3 file system connector: (yes / no / don't know)

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

flinkbot · 2020-02-03T23:21:52Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit af2a640 (Mon Feb 03 23:21:52 UTC 2020)

Warnings:

1 pom.xml files were touched: Check for build and licensing issues.
No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

flinkbot · 2020-02-03T23:45:02Z

CI report:

af2a640 Travis: SUCCESS Azure: FAILURE
6ab5029 Travis: FAILURE Azure: FAILURE
f79154d Travis: FAILURE Azure: FAILURE
6c424da Travis: FAILURE Azure: FAILURE
7e5471a Travis: FAILURE Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build
@flinkbot run azure re-run the last Azure build

carp84

+1

tillrohrmann

Thanks for creating this PR @tweise. I think the change goes in the right direction. However, it does not updates the corresponding NOTICE files. Here is the license guide.

It also affects a lot of modules which rely on jackson-dataformat-cbor such as flink-elasticsearch-base, flink-connector-elasticsearch2, flink-connector-elasticsearch5, flink-connector-elasticsearch7, flink-connector-elasticsearch6, flink-connector-kinesis, flink-sql-connector-elasticsearch7, flink-sql-connector-elasticsearch6 which means that we would need to validate that these modules are still working. I guess you haven't tried them out, right?

Last but not least, I was wondering whether we would need to update other jackson dependencies as well. Currently, we rely on jackson-dataformat-smile, jackson-dataformat-yaml, jackson-module-jaxb-annotations. Would it make sense to bump these dependencies to the same version?

tillrohrmann · 2020-02-04T09:24:14Z

pom.xml

+			<dependency>
+				<groupId>com.fasterxml.jackson.dataformat</groupId>
+				<artifactId>jackson-dataformat-cbor</artifactId>
+				<version>${jackson.version}</version>
 			</dependency>


This change requires the update of all affected NOTICE files.

tillrohrmann · 2020-02-04T09:25:29Z

pom.xml

+			<dependency>
+				<groupId>com.fasterxml.jackson.dataformat</groupId>
+				<artifactId>jackson-dataformat-cbor</artifactId>
+				<version>${jackson.version}</version>
 			</dependency>


Do we really want to change the jackson-dataformat-cbor for all dependencies which rely on it? It would invalidate the testing for all these components.

On the other hand I guess that the functionality used by Kinesis should then also be broken for all the other modules.

What about bumping other jackson dependencies such as jackson-dataformat-smile, jackson-dataformat-yaml, jackson-module-jaxb-annotations? Could they also be affected by the bumping the other dependencies to 2.10.1?

zentol · 2020-02-04T12:29:07Z

pom.xml

+			<!-- https://issues.apache.org/jira/browse/FLINK-15868 -->
+			<dependency>
+				<groupId>com.fasterxml.jackson.dataformat</groupId>
+				<artifactId>jackson-dataformat-cbor</artifactId>


I would only put this into the kinesis connector pom. This is the only module that should be affected by virtue of promoting transitive dependencies via the shade-plugin.
This promotion should likely be removed in the future because it has rather subtle side-effects.

Looks like there is consensus to keep it centralized; I prefer that as well. This dependency isn't specific to Kinesis, it comes from aws-java-sdk-core.

Looks like there is consensus to keep it centralized

Not quite; this came up during the offline discussion and we continued with the existing approach for the time-being for simplicity. Long-term we're likely to throw a jackson bom into every module that needs it, and ban older versions.
This way it is a lot easier to tell which modules are affected by the dependency management.
If a new jackson dependency is added 1 of three cases happen:
a) dependency convergence fails (happens if it is not shaded)
b) enforcer check fails (old insecure versions used that we don't want)
c) a safe version is used and shaded (hence no conflicts), in which case there is no reason to mandate another version. This also allows us to use the version that the transitive dependency was built against, which is one less uncertainty on our side.

tillrohrmann · 2020-02-04T15:38:45Z

After an offline discussion with @zentol and @GJL we decided to bump the remaining Jackson dependencies to version 2.10.1. The affected dependencies are jackson-dataformat-smile, jackson-dataformat-yaml, jackson-module-jaxb-annotations.

I will update the PR to reflect these changes.

tillrohrmann · 2020-02-04T15:53:03Z

We actually don't need to pin jackson-module-jaxb-annotations because we don't use it.

tillrohrmann · 2020-02-04T15:54:50Z

@zentol @GJL, I've updated the PR. Please take a look.

zentol

One minor correction to my code, but otherwise this looks good to me.

zentol · 2020-02-04T16:12:15Z

...ava/org/apache/flink/streaming/connectors/elasticsearch/testutils/SourceSinkDataTestKit.java

 		private static final long serialVersionUID = 1L;

 		private final String index;
+		private XContentBuilderProvider contentBuilderProvider;


my bad, this one should be final

Will update the code.

GJL · 2020-02-04T16:27:41Z

...ava/org/apache/flink/streaming/connectors/elasticsearch/testutils/SourceSinkDataTestKit.java

 		}

 		public IndexRequest createIndexRequest(Tuple2<Integer, String> element) {
 			Map<String, Object> json = new HashMap<>();
 			json.put(DATA_FIELD_NAME, element.f1);

-			return new IndexRequest(index, TYPE_NAME, element.f0.toString()).source(json);
+			try {
+				return new IndexRequest(index, TYPE_NAME, element.f0.toString()).source(contentBuilderProvider.getBuilder().map(json));


I don't think json is accurate here.

Ok I rename it into document.

tillrohrmann · 2020-02-04T17:06:37Z

I've updated the PR and additionally removed the pinning of Jackson dependencies to 2.9.6 from flink-table-planner. Let's hope that everything still runs.

tillrohrmann · 2020-02-04T17:42:19Z

I had to pin snakeyaml to 1.25 to make flink-connector-elasticsearch5 work.

tweise · 2020-02-04T20:31:16Z

LGTM for the additional changes (I cannot approve the PR I opened)

@tillrohrmann thanks for taking care of the updates for the ES modules. We are not using these connectors in our applications, so I have no means to do additional verification. I think it is important to keep the jackson modules on the same version, however. Glad to see this is happening now vs. just patching up the Kinesis connector.

…and jackson-dataformat-cbor

Subsumed by #getUserConfig()

…ch5 to 1.25

…ch5 to 1.25 This closes #11006.

…ch5 to 1.25 This closes apache#11006.

tweise requested a review from zentol February 3, 2020 23:19

rmetzger added the review=description? label Feb 3, 2020

tweise requested a review from GJL February 3, 2020 23:41

rmetzger added the component=Connectors/Kinesis label Feb 4, 2020

carp84 approved these changes Feb 4, 2020

View reviewed changes

tillrohrmann requested changes Feb 4, 2020

View reviewed changes

zentol reviewed Feb 4, 2020

View reviewed changes

tillrohrmann force-pushed the FLINK-15868.jackson-cbor-version branch from af2a640 to aa98860 Compare February 4, 2020 15:54

zentol approved these changes Feb 4, 2020

View reviewed changes

GJL reviewed Feb 4, 2020

View reviewed changes

tillrohrmann force-pushed the FLINK-15868.jackson-cbor-version branch from eddaf63 to f79154d Compare February 4, 2020 17:05

zentol self-assigned this Feb 5, 2020

tweise and others added 8 commits February 5, 2020 11:15

[FLINK-15868][kinesis] Resolve version conflict between jackson-core …

5af8211

…and jackson-dataformat-cbor

[FLINK-15868] Pin jackson-dataformat-smile dependency to 2.10.1

91e95f4

[FLINK-15868] Pin jackson-dataformat-yaml dependency to 2.10.1

e163530

[hotfix][es][tests] Remove unused variable

2edba3c

Subsumed by #getUserConfig()

[hotfix][es][tests] Refactor utils

af8aee0

[FLINK-15868][es] Add tests for different formats

2c8041d

[FLINK-15868] Bump flink-table-planner jackson dependencies to 2.10.1

b1252d3

[FLINK-15868] Pin snakeyaml dependency in flink-connector-elasticsear…

7e5471a

…ch5 to 1.25

tillrohrmann force-pushed the FLINK-15868.jackson-cbor-version branch from 6c424da to 7e5471a Compare February 5, 2020 10:15

tillrohrmann closed this in cf126ff Feb 5, 2020

tillrohrmann added a commit that referenced this pull request Feb 5, 2020

[FLINK-15868] Pin snakeyaml dependency in flink-connector-elasticsear…

81cf2f9

…ch5 to 1.25 This closes #11006.

JTaky pushed a commit to JTaky/flink that referenced this pull request Feb 20, 2020

[FLINK-15868] Pin snakeyaml dependency in flink-connector-elasticsear…

37eeeef

…ch5 to 1.25 This closes apache#11006.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-15868][kinesis] Resolve version conflict between jackson-core and jackson-dataformat-cbor #11006

[FLINK-15868][kinesis] Resolve version conflict between jackson-core and jackson-dataformat-cbor #11006

tweise commented Feb 3, 2020 •

edited

flinkbot commented Feb 3, 2020

flinkbot commented Feb 3, 2020 •

edited

carp84 left a comment

tillrohrmann left a comment •

edited

tillrohrmann Feb 4, 2020

tillrohrmann Feb 4, 2020

tillrohrmann Feb 4, 2020

zentol Feb 4, 2020

tweise Feb 4, 2020

zentol Feb 5, 2020 •

edited

tillrohrmann commented Feb 4, 2020

tillrohrmann commented Feb 4, 2020

tillrohrmann commented Feb 4, 2020

zentol left a comment

zentol Feb 4, 2020

tillrohrmann Feb 4, 2020

GJL Feb 4, 2020

tillrohrmann Feb 4, 2020

tillrohrmann commented Feb 4, 2020

tillrohrmann commented Feb 4, 2020

tweise commented Feb 4, 2020

[FLINK-15868][kinesis] Resolve version conflict between jackson-core and jackson-dataformat-cbor #11006

[FLINK-15868][kinesis] Resolve version conflict between jackson-core and jackson-dataformat-cbor #11006

Conversation

tweise commented Feb 3, 2020 • edited

What is the purpose of the change

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented Feb 3, 2020

Automated Checks

Review Progress

flinkbot commented Feb 3, 2020 • edited

CI report:

carp84 left a comment

Choose a reason for hiding this comment

tillrohrmann left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zentol Feb 5, 2020 • edited

Choose a reason for hiding this comment

tillrohrmann commented Feb 4, 2020

tillrohrmann commented Feb 4, 2020

tillrohrmann commented Feb 4, 2020

zentol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tillrohrmann commented Feb 4, 2020

tillrohrmann commented Feb 4, 2020

tweise commented Feb 4, 2020

tweise commented Feb 3, 2020 •

edited

flinkbot commented Feb 3, 2020 •

edited

tillrohrmann left a comment •

edited

zentol Feb 5, 2020 •

edited