MINOR: remove ZK from code base #529

mjsax · 2024-12-17T04:56:38Z

No description provided.

mjsax · 2024-12-17T04:57:16Z

pom.xml

@@ -241,6 +238,12 @@
            <classifier>test</classifier>
            <scope>test</scope>
        </dependency>
+        <dependency>
+            <groupId>org.apache.kafka</groupId>
+            <artifactId>kafka-test-common</artifactId>


To pull in KafkaClusterTestKit.

mjsax · 2024-12-17T04:57:49Z

pom.xml

+            <groupId>jakarta.xml.bind</groupId>
+            <artifactId>jakarta.xml.bind-api</artifactId>
+            <version>4.0.1</version>
+        </dependency>


W/o this, I got some ClassNotFoundException error -- just Googled for the error and found to add this... 🤷

mjsax · 2024-12-17T04:58:06Z

src/test/java/io/confluent/examples/streams/WikipediaFeedAvroLambdaExampleTest.java

-    producer.send(new ProducerRecord<>(WikipediaFeedAvroExample.WIKIPEDIA_FEED,
-                                       new WikiFeed("john", true, "first post")));
-    producer.flush();
+    try (final KafkaProducer<String, WikiFeed> producer = new KafkaProducer<>(props)) {


Side cleanup to close the producer properly.

mjsax · 2024-12-17T04:58:55Z

src/test/java/io/confluent/examples/streams/kafka/EmbeddedSingleNodeKafkaCluster.java

+    final Map<String, String> effectiveBrokerConfig = effectiveBrokerConfigFrom(brokerConfig);
+    log.debug(
+        "Starting a Kafka instance on port {} ...",
+        effectiveBrokerConfig.get(SocketServerConfigs.LISTENERS_CONFIG)


Given that we don't configure this explicitly any longer, I am not 100% if this is still right?

Have your tried running it?

Yes, I did run it locally -- not sure what your question is?

It does log Starting a Kafka instance on port null ... (what make sense given that we did remove the config...)

I don't know how I could get the port though... maybe best to just remove the DEBUG line all together? Seems not to add much value?

yeah, this is just confusing. remove the log

mjsax · 2024-12-17T05:00:34Z

src/test/java/io/confluent/examples/streams/kafka/EmbeddedSingleNodeKafkaCluster.java

-    effectiveConfig.putAll(brokerConfig);
-    effectiveConfig.put(ZkConfigs.ZK_CONNECT_CONFIG, zookeeper.connectString());
-    effectiveConfig.put(ZkConfigs.ZK_SESSION_TIMEOUT_MS_CONFIG, 30 * 1000);
-    effectiveConfig.put(SocketServerConfigs.LISTENERS_CONFIG, String.format("PLAINTEXT://127.0.0.1:%s", DEFAULT_BROKER_PORT));


Needed to remove this -- otherwise, I got some config errors about other missing config... not sure why this was set to begin with? Did not want to go down the rabbit whole figuring out all other required/dependent configs to keep it (also not sure why it did work with the old setup, but not the new one... 🤷)

mjsax · 2024-12-17T05:01:28Z

src/test/java/io/confluent/examples/streams/kafka/EmbeddedSingleNodeKafkaCluster.java

-      } catch (final Exception fatal) {
-        throw new RuntimeException(fatal);
+      if (schemaRegistry != null) {
+        schemaRegistry.stop();
      }
      if (broker != null) {
        broker.stop();


this throws now, so changed the try-catch to also handle it.

mjsax · 2024-12-17T05:03:02Z

src/test/java/io/confluent/examples/streams/kafka/EmbeddedSingleNodeKafkaCluster.java

@@ -267,16 +253,14 @@ private TopicsDeletedCondition(final String... topics) {

    @Override
    public boolean conditionMet() {
-      //TODO once KAFKA-6098 is fixed use AdminClient to verify topics have been deleted


This is not fixed yet... but was simplest to just use Admin client now...

mjsax · 2024-12-17T05:03:46Z

src/test/java/io/confluent/examples/streams/kafka/KafkaEmbedded.java

-  private Properties effectiveConfigFrom(final Properties initialConfig) {
-    final Properties effectiveConfig = new Properties();
-    effectiveConfig.put(ServerConfigs.BROKER_ID_CONFIG, 0);
-    effectiveConfig.put(SocketServerConfigs.LISTENERS_CONFIG, "PLAINTEXT://127.0.0.1:9092");


As above. Removed this one.

sonarqube-confluent · 2024-12-17T17:56:45Z

Analysis Details

2 Issues

2 Bugs
0 Vulnerabilities
0 Code Smells

Coverage and Duplications

No coverage information (0.00% Estimated after merge)
No duplication information (47.20% Estimated after merge)

Project ID: kafka-streams-examples

View in SonarQube

mjsax · 2024-12-17T22:24:29Z

.semaphore/semaphore.yml

@@ -24,7 +24,7 @@ global_job_config:
  prologue:
    commands:
      - checkout
-      - sem-version java 11
+      - sem-version java 17


Temp fix as ae49f5f did not make it into master yet.

\cc @hk10111 -- hope we can resolve the merge conflicts soon... Or merging this PR will create more conflicts.

lucasbru · 2025-01-03T12:08:48Z

.semaphore/semaphore.yml

@@ -155,7 +155,7 @@ after_pipeline:
      - name: SonarQube
        commands:
          - checkout
-          - sem-version java 11
+          - if [[ $SEMAPHORE_GIT_BRANCH =~ ^7\..* ]]; then sem-version java 8; else sem-version java 17; fi


why change from 11 to 8? 11 is the recommended version for CP.
should we cover 6.x releases as well in case of (out of support window) backports?

For 7.x, we still support Java 8. -- I don't think it's about recommended, but supported?

It's more a question for @hk10111 thouhg. Cf #527

I would have hard-coded Java 8 for older branches and hard-coded Java 17 for 8.0+, but they want to have the same semaphore.yml config across all branches -- that's why the if-else.

Guess we could also use Java 11 for older branches, but historically we use Java 8.

Anyway, the other commit finally made it into all branches I believe, so this change will go away for this PR anyway.

lucasbru

I left some comments, but mostly looking good to me

mjsax · 2025-01-07T02:43:36Z

Build is broken due to broken upstream dependency. Need to wait for an upstream fix...

mjsax requested a review from a team as a code owner December 17, 2024 04:56

This comment has been minimized.

Sign in to view

mjsax commented Dec 17, 2024

View reviewed changes

airlock-confluentinc bot force-pushed the remove-zk branch from 2338aa8 to 241652b Compare December 23, 2024 17:06

mjsax added 6 commits December 23, 2024 09:54

MINOR: remove ZK from code base

f140aaa

bump Java to 17

151bc6a

Update JDK to 17

83c4ae1

fix tests

16607c9

Remove algebird example

29375cc

update readme

d79f036

airlock-confluentinc bot force-pushed the remove-zk branch from 8c2b34b to d79f036 Compare December 23, 2024 17:54

mjsax added 2 commits December 23, 2024 09:56

update

e289bd1

readme

ab6f948

lucasbru reviewed Jan 3, 2025

View reviewed changes

lucasbru approved these changes Jan 3, 2025

View reviewed changes

remvove log

64e7ec7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MINOR: remove ZK from code base #529

MINOR: remove ZK from code base #529

mjsax commented Dec 17, 2024

This comment has been minimized.

mjsax Dec 17, 2024

mjsax Dec 17, 2024

mjsax Dec 17, 2024

mjsax Dec 17, 2024

lucasbru Jan 3, 2025

mjsax Jan 3, 2025

mjsax Jan 3, 2025

lucasbru Jan 7, 2025

mjsax Dec 17, 2024

mjsax Dec 17, 2024

mjsax Dec 17, 2024

mjsax Dec 17, 2024

sonarqube-confluent bot commented Dec 17, 2024

mjsax Dec 17, 2024

lucasbru Jan 3, 2025 •

edited

Loading

mjsax Jan 3, 2025

lucasbru left a comment

mjsax commented Jan 7, 2025

MINOR: remove ZK from code base #529

Are you sure you want to change the base?

MINOR: remove ZK from code base #529

Conversation

mjsax commented Dec 17, 2024

This comment has been minimized.

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

sonarqube-confluent bot commented Dec 17, 2024

Analysis Details

2 Issues

Coverage and Duplications

Choose a reason for hiding this comment

lucasbru Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasbru left a comment

Choose a reason for hiding this comment

mjsax commented Jan 7, 2025

lucasbru Jan 3, 2025 •

edited

Loading