[FLINK-24765][kafka] Bump Kafka version to 2.8 #17696

fapaul · 2021-11-05T12:55:50Z

What is the purpose of the change

With dropping Scala 2.11 we can now finally upgrade our Kafka dependencies which is very important to ensure the stability of our tests because we were missing a few years of Kafka fixes.

Brief change log

Bump Kafka version to 2.8

Verifying this change

All tests are still passing

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, Kubernetes/Yarn, 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 · 2021-11-05T12:58:51Z

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 ebf1f24 (Fri Nov 05 12:58:51 UTC 2021)

Warnings:

2 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 · 2021-11-05T12:59:46Z

CI report:

f0568b0 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

MartijnVisser

Did a quick review (I'm no Java export so probably someone else should have a more thorough look). Just a couple of questions, looks good to me overall

MartijnVisser · 2021-11-05T13:15:45Z

flink-connectors/flink-connector-kafka/pom.xml

+		<!-- Required to execute the kafka server for testing. Please change the zookeeper version accordingly when changing the Kafka version
+             https://github.com/apache/kafka/blob/839b886f9b732b151e1faeace7303c80641c08c4/gradle/dependencies.gradle#L122 -->
+		<dependency>
+			<groupId>org.apache.zookeeper</groupId>
+			<artifactId>zookeeper</artifactId>
+			<version>3.5.9</version>
+			<scope>test</scope>
+		</dependency>
+


I believe @dmvk was looking into dropping Zookeeper 3.4 for 1.15, so would that also solve this issue then?

If Kafka needs a specific Zookeeper version then it is better to specify that in the pom; otherwise this can easily break if we change the Zookeeper version.

Whether we drop ZK 3.4 or not is largely independent from this in any case, because that primarily applies to flink-shaded-zookeeper.

It would actually be interesting to know which modules actually rely on vanilla zookeeper; maybe the root pom entry isn't that useful anymore.

Honestly, I am not sure whether other versions suffice here. I just know that the kafka-server no longer has this dependency therefore we have to include it. I guess other zookeeper versions will also work but this is the one currently included by Kafka therefore I left the link to check the corresponding version.

@MartijnVisser for more context on the zookeeper bump - https://issues.apache.org/jira/browse/FLINK-24707

MartijnVisser · 2021-11-05T13:21:45Z

...s-parent/flink-test-utils-junit/src/main/java/org/apache/flink/util/DockerImageVersions.java

@@ -33,7 +33,7 @@
    public static final String ELASTICSEARCH_6 =
            "docker.elastic.co/elasticsearch/elasticsearch-oss:6.3.1";

-    public static final String KAFKA = "confluentinc/cp-kafka:5.5.2";
+    public static final String KAFKA = "confluentinc/cp-kafka:6.2.1";


Shouldn't we also update https://github.com/fapaul/flink/blob/FLINK-24765/flink-end-to-end-tests/flink-end-to-end-tests-common-kafka/src/test/java/org/apache/flink/tests/util/kafka/KafkaSourceE2ECase.java#L37 and https://github.com/fapaul/flink/blob/FLINK-24765/flink-connectors/flink-connector-kafka/src/test/java/org/apache/flink/connector/kafka/source/KafkaSourceITCase.java#L248 ?

I've already run into this in the past, would it be possible to unify these (a single constant shared between these tests)?

MartijnVisser · 2021-11-05T13:22:40Z

...n/java/org/apache/flink/streaming/connectors/kafka/internals/FlinkKafkaInternalProducer.java

-    @Override
-    public void close(long timeout, TimeUnit unit) {
-        synchronized (producerClosingLock) {
-            kafkaProducer.close(timeout, unit);
-            if (LOG.isDebugEnabled()) {
-                LOG.debug(
-                        "Closed internal KafkaProducer {}. Stacktrace: {}",
-                        System.identityHashCode(this),
-                        Joiner.on("\n").join(Thread.currentThread().getStackTrace()));
-            }
-            closed = true;
-        }
-    }
-


Don't we need this anymore or is this no longer possible?

The method is no longer part of the interface therefore we cannot implement it anymore.

Was it dead code before? (looks to me like it)

Why should be it dead code?

Ah sorry, I looked at the wrong commit.

Kafka didn't remove this method in 2.8.1, it removed it in 3.0.0
https://github.com/apache/kafka/blob/2.8/clients/src/main/java/org/apache/kafka/clients/producer/Producer.java#L104

Thanks for the hint during the upgrade test I also tested with 3.0.0. Probably I missed that when going back to 2.8.

By bumping the kafka version to the latest 2.x release we include the latest patches of Kafka which should harden our tests.

…r image

MartijnVisser

Looking good to me! Thank you

AHeise · 2021-11-11T07:17:59Z

...afka/src/test/java/org/apache/flink/streaming/connectors/kafka/FlinkKafkaProducerITCase.java

+ * <p>Do not run this class in the same junit execution with other tests in your IDE. This may lead
+ * leaking threads.


Is this an issue because tests may be run in parallel in the same JVM of the IDE and thus the leak detection looks at the threads of another test?

If we see more tests than that, we could also introduce an annotation later.

Is this an issue because tests may be run in parallel in the same JVM of the IDE and thus the leak detection looks at the threads of another test?

Are tests within the same module by default run in parallel? Perhaps there is also another test within the module leaking thread that is executed before the FlinkKafkaProducerITCase ...

If we see more tests than that, we could also introduce an annotation later.

Good idea, hopefully, this case does not become more often

MartijnVisser reviewed Nov 5, 2021

View reviewed changes

rmetzger added the component=Connectors/Kafka label Nov 5, 2021

fapaul force-pushed the FLINK-24765 branch from ebf1f24 to 8c9f16f Compare November 8, 2021 08:43

Fabian Paul added 3 commits November 8, 2021 14:16

[FLINK-24765][kafka] Bump Kafka version to 2.8

21e264f

By bumping the kafka version to the latest 2.x release we include the latest patches of Kafka which should harden our tests.

[FLINK-24765][kafka] Mark tests which are unstable in the IDE

776875a

[FLINK-24765][tests] Use DockerImageVersions to reference Kafka docke…

f0568b0

…r image

fapaul force-pushed the FLINK-24765 branch from 8c9f16f to f0568b0 Compare November 8, 2021 13:16

MartijnVisser approved these changes Nov 8, 2021

View reviewed changes

AHeise reviewed Nov 11, 2021

View reviewed changes

AHeise merged commit e61ff88 into apache:master Nov 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-24765][kafka] Bump Kafka version to 2.8 #17696

[FLINK-24765][kafka] Bump Kafka version to 2.8 #17696

fapaul commented Nov 5, 2021

flinkbot commented Nov 5, 2021

flinkbot commented Nov 5, 2021 •

edited

Loading

MartijnVisser left a comment

MartijnVisser Nov 5, 2021

zentol Nov 5, 2021

fapaul Nov 5, 2021

dmvk Nov 8, 2021

MartijnVisser Nov 5, 2021

dmvk Nov 8, 2021

MartijnVisser Nov 5, 2021

fapaul Nov 8, 2021

AHeise Nov 10, 2021

fapaul Nov 10, 2021

AHeise Nov 11, 2021

ijuma Jan 22, 2022 •

edited

Loading

fapaul Jan 24, 2022

MartijnVisser left a comment

AHeise Nov 11, 2021

AHeise Nov 11, 2021

fapaul Nov 11, 2021

fapaul Nov 11, 2021

		* <p>Do not run this class in the same junit execution with other tests in your IDE. This may lead
		* leaking threads.

[FLINK-24765][kafka] Bump Kafka version to 2.8 #17696

[FLINK-24765][kafka] Bump Kafka version to 2.8 #17696

Conversation

fapaul commented Nov 5, 2021

What is the purpose of the change

Brief change log

Verifying this change

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

Documentation

flinkbot commented Nov 5, 2021

Automated Checks

Review Progress

flinkbot commented Nov 5, 2021 • edited Loading

CI report:

MartijnVisser 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

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

Choose a reason for hiding this comment

ijuma Jan 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartijnVisser 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

flinkbot commented Nov 5, 2021 •

edited

Loading

ijuma Jan 22, 2022 •

edited

Loading