MINOR: fix broken master #448

mjsax · 2023-10-05T03:36:21Z

StreamsResetter got some methods renamed breaking the build.

In addition, this PR cleansup warning, and migrates off deprecated APIs.

StreamsResetter got some methods renamed breaking the build. In addition, this PR cleansup warning, and migrates off deprecated APIs.

cla-assistant · 2023-10-05T03:36:34Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

cla-assistant · 2023-10-05T03:36:35Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

mjsax · 2023-10-05T03:37:12Z

src/main/java/io/confluent/examples/streams/AnomalyDetectionLambdaExample.java

-    streamsConfiguration.put(StreamsConfig.DEFAULT_KEY_SERDE_CLASS_CONFIG, Serdes.String().getClass().getName());
-    streamsConfiguration.put(StreamsConfig.DEFAULT_VALUE_SERDE_CLASS_CONFIG, Serdes.String().getClass().getName());
+    streamsConfiguration.put(StreamsConfig.DEFAULT_KEY_SERDE_CLASS_CONFIG, Serdes.StringSerde.class);
+    streamsConfiguration.put(StreamsConfig.DEFAULT_VALUE_SERDE_CLASS_CONFIG, Serdes.StringSerde.class);


Warning that the created Serde instance might not be closed -- we actually don't need to create an object to get the class though.

mjsax · 2023-10-05T03:37:39Z

src/main/java/io/confluent/examples/streams/AnomalyDetectionLambdaExample.java

@@ -134,7 +134,7 @@ public static void main(final String[] args) {
      // count users, using one-minute tumbling windows;
      // no need to specify explicit serdes because the resulting key and value types match our default serde settings
      .groupByKey()
-      .windowedBy(TimeWindows.of(Duration.ofMinutes(1)))
+      .windowedBy(TimeWindows.ofSizeWithNoGrace(Duration.ofMinutes(1)))


Deprecated -- by default I switch to use "no grace".

mjsax · 2023-10-05T03:38:09Z

src/main/java/io/confluent/examples/streams/GlobalStoresExample.java

-            }
-
-            @Override
-            public void close() {


close() has a default impl now and we can remove it.

mjsax · 2023-10-05T03:38:31Z

src/main/java/io/confluent/examples/streams/AnomalyDetectionLambdaExample.java

@@ -40,7 +40,7 @@
 * <br>
 * HOW TO RUN THIS EXAMPLE
 * <p>
- * 1) Start Zookeeper and Kafka. Please refer to <a href='http://docs.confluent.io/current/quickstart.html#quickstart'>QuickStart</a>.
+ * 1) Start Zookeeper and Kafka. Please refer to <a href="http://docs.confluent.io/current/quickstart.html#quickstart">QuickStart</a>.


Just some HTML cleanup.

mjsax · 2023-10-05T03:39:29Z

src/main/java/io/confluent/examples/streams/SessionWindowsExample.java

@@ -164,7 +164,7 @@ static Topology buildTopology(final Map<String, String> serdeConfig) {
        // group by key so we can count by session windows
        .groupByKey(Grouped.with(Serdes.String(), playEventSerde))
        // window by session
-        .windowedBy(SessionWindows.with(INACTIVITY_GAP))
+        .windowedBy(SessionWindows.ofInactivityGapAndGrace(INACTIVITY_GAP, Duration.ofMillis(100)))


For this case, we need a grace to make the test pass -- without a grace, store history is limited and cleaned up too quickly, and IQ that is used in the test cannot find the data any longer.

mjsax · 2023-10-05T03:40:31Z

...main/java/io/confluent/examples/streams/interactivequeries/kafkamusic/KafkaMusicExample.java

@@ -456,31 +429,26 @@ public TopFiveSongs deserialize(final String s, final byte[] bytes) {
          }
          return result;
        }
-
-        @Override
-        public void close() {


Has a default impl now and can be removed.

mjsax · 2023-10-05T03:41:26Z

src/main/java/io/confluent/examples/streams/microservices/OrdersService.java

+        continue;
+      } catch (final NotFoundException swallow) {
+        // swallow
+      }


This is an actually rewrite -- hope I got it right...

mjsax · 2023-10-05T03:42:07Z

src/main/java/io/confluent/examples/streams/microservices/OrdersService.java

@@ -248,11 +247,6 @@ private HostStoreInfo getKeyLocationOrBlock(final String id, final AsyncResponse
    return locationOfKey;
  }

-  private boolean locationMetadataIsUnavailable(final HostStoreInfo hostWithKey) {


This method does not work any longer because NOT_AVAILABLE is not available any more -- triggered the rewrite from above

mjsax · 2023-10-05T03:42:42Z

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

@@ -134,7 +134,7 @@ public void shouldReprocess() throws Exception {
    }

    // reset application
-    final int exitCode = new StreamsResetter().run(
+    final int exitCode = new StreamsResetter().execute(


This method was renamed what broke the build.

FYI: apache/kafka#13983 (comment)
Using the StreamsResetter class directly is actually not the intended use.

We use StreamsResetter directly in multiple places like the benchmarks, I think. So I am fine with it. That also means that we might run into breaking changes once in a while.

mjsax · 2023-10-05T03:45:09Z

.../java/io/confluent/examples/streams/interactivequeries/kafkamusic/KafkaMusicExampleTest.java

@@ -241,7 +241,9 @@ public void shouldCreateChartsAndAccessThemViaInteractiveQueries() throws Except
        final ReadOnlyKeyValueStore<Long, Song> songsStore;
        try {
          songsStore = streams.store(fromNameAndType(KafkaMusicExample.ALL_SONGS, QueryableStoreTypes.keyValueStore()));
-          return songsStore.all().hasNext();
+          try (final KeyValueIterator<Long, Song> it = songsStore.all()) {


Iterator must be closed to avoid resource leak.

cadonna

Thanks for the PR, @mjsax !

Here my feedback!

cadonna · 2023-10-05T16:59:19Z

src/main/java/io/confluent/examples/streams/GlobalStoresExample.java

-import org.apache.kafka.streams.processor.api.Processor;
-import org.apache.kafka.streams.processor.api.ProcessorContext;
-import org.apache.kafka.streams.processor.api.Record;
+import org.apache.kafka.streams.processor.api.*;


We usually do not use wildcard imports, do we?

Ah. IntelliJ... Fixing.

cadonna · 2023-10-05T17:02:12Z

...main/java/io/confluent/examples/streams/interactivequeries/kafkamusic/KafkaMusicExample.java

      };
    }
  }

  /**
   * Used in aggregations to keep track of the Top five songs
   *
-   * Warning: this aggregator relies on the current order of execution
+   * <p></p>Warning: this aggregator relies on the current order of execution


Should this be only <p> ?

cadonna · 2023-10-05T17:03:46Z

src/main/java/io/confluent/examples/streams/microservices/OrdersService.java

-import javax.ws.rs.PathParam;
-import javax.ws.rs.Produces;
-import javax.ws.rs.QueryParam;
+import javax.ws.rs.*;


See my comment above about wildcard imports.

cadonna · 2023-10-05T17:12:12Z

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

@@ -134,7 +134,7 @@ public void shouldReprocess() throws Exception {
    }

    // reset application
-    final int exitCode = new StreamsResetter().run(
+    final int exitCode = new StreamsResetter().execute(


FYI: apache/kafka#13983 (comment)
Using the StreamsResetter class directly is actually not the intended use.

cadonna

LGTM!

Thanks @mjsax

lucasbru

lgtm, thanks!

MINOR: fix broken master

11b4e50

StreamsResetter got some methods renamed breaking the build. In addition, this PR cleansup warning, and migrates off deprecated APIs.

mjsax requested a review from a team as a code owner October 5, 2023 03:36

cleanup

a96dc57

mjsax commented Oct 5, 2023

View reviewed changes

cadonna reviewed Oct 5, 2023

View reviewed changes

Review comments

249dca7

cadonna approved these changes Oct 6, 2023

View reviewed changes

lucasbru approved these changes Oct 6, 2023

View reviewed changes

mjsax merged commit e9494c9 into master Oct 6, 2023
3 of 4 checks passed

mjsax deleted the fix-master branch October 6, 2023 20:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MINOR: fix broken master #448

MINOR: fix broken master #448

mjsax commented Oct 5, 2023

cla-assistant bot commented Oct 5, 2023

cla-assistant bot commented Oct 5, 2023

mjsax Oct 5, 2023

mjsax Oct 5, 2023

mjsax Oct 5, 2023

mjsax Oct 5, 2023

mjsax Oct 5, 2023

mjsax Oct 5, 2023

mjsax Oct 5, 2023

mjsax Oct 5, 2023

mjsax Oct 5, 2023

cadonna Oct 5, 2023

cadonna Oct 5, 2023

mjsax Oct 5, 2023

cadonna left a comment

cadonna Oct 5, 2023

mjsax Oct 5, 2023

cadonna Oct 5, 2023

cadonna Oct 5, 2023

cadonna Oct 5, 2023

cadonna left a comment

lucasbru left a comment

MINOR: fix broken master #448

MINOR: fix broken master #448

Conversation

mjsax commented Oct 5, 2023

cla-assistant bot commented Oct 5, 2023

cla-assistant bot commented Oct 5, 2023

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

cadonna 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

cadonna left a comment

Choose a reason for hiding this comment

lucasbru left a comment

Choose a reason for hiding this comment