upgraded_kafka_client_version_to_3.9.0 by adkharat · Pull Request #279 · prestodb/tempto

adkharat · 2024-11-27T04:42:09Z

Existing kafka version = 0.11.0.2
Upgraded Kafka version = 3.9.0

With reference to prestodb/presto PR Fix CVE-2022-34917

Kafka 3.9.0 depricates direct Zookeeper interactions:

ZkClient,
ZkUtils

Upgrading depricates below AdminUtils methods:

AdminUtils.createTopic()
AdminUtils.topicExists()
AdminUtils.deleteTopic()

Deprecated AdminUtils functions are replaced with AdminClient APIs
Deprecated Zookeeper dependency is replaced with Kafka's AdminClient API.
Encapsulated AdminClient logic in a method KafkaAdminClient to manage lifecycle and reduce boilerplate.
Operations such as topic creation (createTopic) and deletion (deleteTopic) now use AdminClient's createTopics and deleteTopics.

FYI:

kafka.utils.ZkUtils was deprecated since 2.0.0.

2.0.0- > ZKUtils present
2.3.1 -> ZKUtils present
2.4.0 -> ZKUtils removed
2.8.2 -> ZKUtils not present (Version 2.8.2 has some vulnerability)
3.7.1 -> ZKUtils not present (No vulnerability)
3.9.0 -> ZKUtils not present (Latest version + No vulnerability)

adkharat · 2024-11-27T06:00:19Z

Build on

gradle -v                                                     
------------------------------------------------------------
Gradle 6.6
------------------------------------------------------------

Build time:   2020-08-10 22:06:19 UTC
Revision:     d119144684a0c301aea027b79857815659e431b9

Kotlin:       1.3.72
Groovy:       2.5.12
Ant:          Apache Ant(TM) version 1.10.8 compiled on May 10 2020
JVM:          1.8.0_422 (Azul Systems, Inc. 25.422-b05)
OS:           Mac OS X 14.4 x86_64

adkharat · 2024-11-27T06:04:47Z

@imjalpreet can you please review the changes.

imjalpreet · 2024-12-05T23:07:08Z

@ZacBlanco FYI

ZacBlanco · 2024-12-06T18:33:13Z

tempto-examples/build.gradle


 shadowJar {
    version = ''
+    zip64 = true


why is this required?

@ZacBlanco on gradle build, I am getting error

Execution failed for task ':tempto-examples:shadowJar'. > shadow.org.apache.tools.zip.Zip64RequiredException: archive contains more than 65535 entries.

Here, ShadowJar plugin is creating a JAR file that exceeds the limit of 65,535 entries. This might due to the ZIP format limitation that ShadowJar uses by default. Link

Got it, thanks for the explanation. This is fine then I guess. We were probably close to the limit and the newer version pushed us over? It would be good to know the number of entries before/after this change, but I don't think it's worth trying to fix

ZacBlanco

Only other concern is the same with Jalpreet's on the presto PR. I think we should try to move to the latest available Kafka version

adkharat · 2024-12-06T19:49:39Z

@ZacBlanco Latest Kafka version is 3.9.0.
Pushed changes for 3.9.0

ZacBlanco

Just one comment, otherwise LGTM.

ZacBlanco · 2024-12-10T18:33:36Z

build.gradle

 ext.tempto_kafka = project(':tempto-kafka')
 ext.expected_result_generator = project(':expected-result-generator')
-ext.tempto_version = '1.53'
+ext.tempto_version = '1.54'


This should only be upgraded and set to a non-SNAPSHOT version during the release process. It looks like after the last release it wasn't properly updated. Since we already release 1.53, this should be 1.54-SNAPSHOT. During release we will update to 1.54, and then move to 1.55-SNAPSHOT afterwards.

So could you set this to 1.54-SNAPSHOT for now?

@ZacBlanco Done. Updated to 1.54-SNAPSHOT

adkharat · 2024-12-12T03:45:22Z

@ZacBlanco I don't see an option to merge the PR. If no additional approvals are required, could you please merge these PR?
I believe the Tempto jar should get auto-published to https://oss.sonatype.org/#nexus-search;quick~io.prestodb.tempto based on the configuration in build.gradle

ZacBlanco · 2024-12-12T06:00:10Z

I can't merge it. I don't have write access to this repository. @imjalpreet if you have time could you also review?

The jar is not automatically published. Someone with write access has to publish it to maven central. We also need to verify that the latest jar update doesn't break the presto-product-tests CI on the presto repository.

@tdcmeehan can merge after we get Jalpreet's review and verify that the CI does not break.

agrawalreetika · 2024-12-16T19:49:32Z

build.gradle

 ext.tempto_kafka = project(':tempto-kafka')
 ext.expected_result_generator = project(':expected-result-generator')
-ext.tempto_version = '1.53'
+ext.tempto_version = '1.54-SNAPSHOT'


I think, before there should be a release prep commit to update ext.tempto_version from 1.53 to 1.54-SNAPSHOT? something like -
c1eec18

Then in your PR, you should change version from 1.54-SNAPSHOT to 1.54 for next release.

I think, before there should be a release prep commit to update ext.tempto_version from 1.53 to 1.54-SNAPSHOT? something like - c1eec18

Then in your PR, you should change version from 1.54-SNAPSHOT to 1.54 for next release.

@agrawalreetika

I checked the previous commits and can confirmed that no commits were pushed earlier for 1.54-SNAPSHOT.
From an example I found, the process involves first merging 1.54-SNAPSHOT and then creating another PR on top of it for 1.54.

Here are the previous PRs for reference:

Prepare 1.53 development release iteration #275

Prepare 1.53 release #276

Let me know, Shall I create a new PR for 1.54-SNAPSHOT ? and once it's merged, 1.54 will be updated with this current PR.

agrawalreetika · 2024-12-16T20:09:30Z

build.gradle

        kafka_clients        : "org.apache.kafka:kafka-clients:${versions.kafka}",
-        kafka                : "org.apache.kafka:kafka_2.12:${versions.kafka}",
-        zkclient             : "com.101tec:zkclient:${versions.zkclient}"
+        kafka                : "org.apache.kafka:kafka_2.12:${versions.kafka}"


Curious, which class is getting used from this library?

Classes with package names starting with org.apache.kafka.clients will originate from the kafka_clients library.

For example:-

import org.apache.kafka.clients.producer.KafkaProducer; import org.apache.kafka.clients.admin.AdminClient;

Classes with package names starting with org.apache.kafka.common will originate from the kafka library.

For example:-

import org.apache.kafka.common.errors.TopicExistsException; import org.apache.kafka.common.serialization.ByteArraySerializer

Both are being used in KafkaTableManager.java

Hi @adkharat

I just checked I see below classes that you mentioned are coming from kafka-clients-3.9.0.jar nothing from these are from org.apache.kafka:kafka_2.12:${versions.kafka}

org/apache/kafka/clients/producer/KafkaProducer.class org/apache/kafka/clients/admin/AdminClient.class org/apache/kafka/common/errors/TopicExistsException.class org/apache/kafka/common/serialization/ByteArraySerializer.class

So I am curious do we even need "org.apache.kafka:kafka_2.12:${versions.kafka}" in this upgrade?

@agrawalreetika
That is the case then not required.
But kafka: "org.apache.kafka:kafka_2.12:${versions.kafka} is referenced in the tempto-kafka/build.gradle file. Build will fails if it's removed.
I think, in tempto-kafka/build.gradle file, we need to replace the reference to libraries.kafka with libraries.kafka_clients. Then we are good to remove.
I have pushed changes.

agrawalreetika · 2024-12-16T20:39:53Z

tempto-kafka/src/main/java/io/prestodb/tempto/fulfillment/table/kafka/KafkaTableManager.java

+        Properties properties = new Properties();
+        properties.put(AdminClientConfig.BOOTSTRAP_SERVERS_CONFIG, brokerHost + ":" + brokerPort);
+        AdminClient adminClient = AdminClient.create(properties);
+        try {


We can use try-with-resources instead for handling close of adminClient

agrawalreetika · 2024-12-16T20:40:16Z

tempto-kafka/src/main/java/io/prestodb/tempto/fulfillment/table/kafka/KafkaTableManager.java

+        properties.put(AdminClientConfig.BOOTSTRAP_SERVERS_CONFIG, brokerHost + ":" + brokerPort);
+        AdminClient adminClient = AdminClient.create(properties);
+        try {
+            routine.accept(adminClient);


What would happen if there is an issue/exception while doing above? Should we add a catch and logger for it?

@agrawalreetika added catch to handle exception and logged error.

tdcmeehan

Please rebase to pick up the GitHub actions job.

adkharat · 2024-12-17T07:41:54Z

Please rebase to pick up the GitHub actions job.

@tdcmeehan Done. Rebased

upgraded_kafka_client_version_to_3.9.0 upgraded_kafka_client_version_to_2.8.2 undo commented code removed zkclient Upgraded kafka version to 3.7.1 upgraded to 3.9.0 updated version to 1.54-SNAPSHOT Prepare 1.54 development release iteration upgraded_kafka_client_version_to_3.9.0 added topic check before deletion added retry in topic deletion

ZacBlanco · 2025-04-22T19:52:53Z

@adkharat I am closing this since we merged #288

adkharat force-pushed the upgrade_kafka_client_version_cve_2022_34917 branch from 2576948 to a15a170 Compare November 27, 2024 05:58

adkharat marked this pull request as ready for review November 27, 2024 06:00

adkharat changed the title ~~upgraded_kafka_client_version_to_2.8.2~~ upgraded_kafka_client_version_to_3.7.1 Dec 2, 2024

ZacBlanco reviewed Dec 6, 2024

View reviewed changes

adkharat changed the title ~~upgraded_kafka_client_version_to_3.7.1~~ upgraded_kafka_client_version_to_3.9.0 Dec 6, 2024

adkharat force-pushed the upgrade_kafka_client_version_cve_2022_34917 branch from d2d3660 to 6bf9291 Compare December 6, 2024 20:00

ZacBlanco requested changes Dec 10, 2024

View reviewed changes

adkharat force-pushed the upgrade_kafka_client_version_cve_2022_34917 branch 2 times, most recently from c6a547a to 411394e Compare December 11, 2024 10:37

ZacBlanco approved these changes Dec 11, 2024

View reviewed changes

agrawalreetika requested changes Dec 16, 2024

View reviewed changes

tdcmeehan requested changes Dec 16, 2024

View reviewed changes

adkharat force-pushed the upgrade_kafka_client_version_cve_2022_34917 branch 3 times, most recently from 10ff64e to ba6b9dc Compare December 17, 2024 07:37

adkharat requested review from agrawalreetika and tdcmeehan December 17, 2024 08:02

adkharat force-pushed the upgrade_kafka_client_version_cve_2022_34917 branch 3 times, most recently from 0603fa5 to f364578 Compare December 18, 2024 04:36

adkharat force-pushed the upgrade_kafka_client_version_cve_2022_34917 branch from 203cbe8 to 132bb5a Compare December 18, 2024 05:19

ZacBlanco closed this Apr 22, 2025

Conversation

adkharat commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adkharat commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adkharat commented Nov 27, 2024

Uh oh!

imjalpreet commented Dec 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZacBlanco Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZacBlanco left a comment

Choose a reason for hiding this comment

Uh oh!

adkharat commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZacBlanco left a comment

Choose a reason for hiding this comment

Uh oh!

ZacBlanco Dec 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adkharat commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZacBlanco commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agrawalreetika Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adkharat Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adkharat Dec 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agrawalreetika Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdcmeehan left a comment

Choose a reason for hiding this comment

Uh oh!

adkharat commented Dec 17, 2024

Uh oh!

ZacBlanco commented Apr 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

adkharat commented Nov 27, 2024 •

edited

Loading

adkharat commented Nov 27, 2024 •

edited

Loading

ZacBlanco Dec 6, 2024 •

edited

Loading

adkharat commented Dec 6, 2024 •

edited

Loading

ZacBlanco Dec 10, 2024 •

edited

Loading

adkharat commented Dec 12, 2024 •

edited

Loading

ZacBlanco commented Dec 12, 2024 •

edited

Loading

agrawalreetika Dec 16, 2024 •

edited

Loading

adkharat Dec 17, 2024 •

edited

Loading

adkharat Dec 17, 2024 •

edited

Loading

agrawalreetika Dec 16, 2024 •

edited

Loading