Use ValueTransformerWithKey instead of Transformer by inponomarev · Pull Request #292 · confluentinc/kafka-tutorials

Ivan Ponomarev (inponomarev) · 2020-03-19T15:26:15Z

Hello,

the current version of the tutorial uses Transformer. However, since Transformer may change the key, its usage will lead to redundant repartitioning when grouping operations are used aftewards.

I propose to rewrite this example using ValueTransformer[WithKey], which will not cause the unwanted repartitioning.

By the way, it seems to me that we have a bug in transformValues: see the filter((k, v) -> v != null) after it. Without this filter, null values appear downstream, contrary to what is said in JavaDoc:

If the return value of ValueTransformerWithKey#transform(Object, Object) is null, no records are emitted.

Ivan Ponomarev (inponomarev) · 2020-03-19T15:29:12Z

Concerning the need of filter: it seems to be KAFKA-9533, which is going to be fixed soon.

Bill Bejeck (bbejeck) · 2020-03-26T17:21:25Z

Hi Ivan Ponomarev (@inponomarev),

Thanks for the PR.

Yes, you are correct in the usage of a Transformer vs. TransformValues, but since the tutorial is working as intended what do you think about adding your comments above to the _includes/tutorials/finding-distinct/kstreams/markup/dev/make-topology.adoc file?

Concerning the need of filter: it seems to be KAFKA-9533, which is going to be fixed soon

Thanks for pointing that out. But it seems the JavaDocs were in error, and this merged PR apache/kafka#8298 corrects the issue. So you'll still need to use a filter operator to eliminate any null values that result.

Ivan Ponomarev (inponomarev) · 2020-04-07T12:12:15Z

Hi Bill Bejeck (@bbejeck) sorry for delay with this, I have updated the tutorial.

Concerning the nulls in TransformValues, I found out that in our code we have cases when we emit nulls from TransformValues explicitly (in order to delete values from tables afterwards), so fixing the Javadoc and leaving TransformValues behaviour unchanged seems to be the most convenient way.

Bill Bejeck (bbejeck)

Thanks, Ivan Ponomarev (@inponomarev) looks good overall some text updates are needed for rendering purposes.

Bill Bejeck (bbejeck) · 2020-04-07T15:24:09Z

-Focusing on the `buildTopology` method, note how the Kafka Streams topology relies on a `https://docs.confluent.io/current/streams/javadocs/org/apache/kafka/streams/kstream/Transformer.html[Transformer]` and a `https://docs.confluent.io/current/streams/javadocs/org/apache/kafka/streams/state/WindowStore.html[Window Store]` to filter out the duplicate IP addresses.   Events are de-duped within a 2 minute window, and unique clicks are produced to a new topic named `distinct-clicks`.
+Focusing on the `buildTopology` method, note how the Kafka Streams topology relies on a `https://docs.confluent.io/current/streams/javadocs/org/apache/kafka/streams/kstream/ValueTransformerWithKey.html[ValueTransformerWithKey]` and a `https://docs.confluent.io/current/streams/javadocs/org/apache/kafka/streams/state/WindowStore.html[Window Store]` to filter out the duplicate IP addresses.   Events are de-duped within a 2 minute window, and unique clicks are produced to a new topic named `distinct-clicks`.
+
+NOTE: Note that we are using `https://docs.confluent.io/current/streams/javadocs/org/apache/kafka/streams/kstream/ValueTransformerWithKey.html[ValueTransformerWithKey]` here instead of `https://docs.confluent.io/current/streams/javadocs/org/apache/kafka/streams/kstream/Transformer.html[Transformer]` since we need keys to transform data, but there is no need to re-key the stream. 


Unfortunately, call-outs don't render correctly at the moment. Can you remove the NOTE: text? Maybe you could do something like **_Note that we are using_** instead. We are working on adding support for call-outs.

Oh I see! I got used to Asciidoctor in my own projects, that's why I'm using its features without thinking about render support :-)

Bill Bejeck (bbejeck) · 2020-04-07T16:35:25Z

+        Runtime.getRuntime().addShutdownHook(new Thread("streams-shutdown-hook") {
+            @Override
+            public void run() {
+                streams.close();


Suggestion - I know this is pre-existing code, but can we update close() to close(Duration.ofSeconds(5))

Bill Bejeck (bbejeck) · 2020-04-07T16:36:12Z

validated by going through the tutorial steps and running the test harness locally.

Ivan Ponomarev (inponomarev) · 2020-04-07T17:18:29Z

Bill Bejeck (@bbejeck) done as you suggested, apparently I also forgot to commit build.gradle with version updates. Will you please re-check it with newer versions, 'it works on my machine' 😄

Bill Bejeck (bbejeck) · 2020-04-07T17:41:22Z

Bill Bejeck (@bbejeck) done as you suggested, apparently I also forgot to commit build.gradle with version updates. Will you please re-check it with newer versions, 'it works on my machine' 😄

Ack and will do. Thanks for the quick response.

Bill Bejeck (bbejeck)

Ivan Ponomarev (@inponomarev) I tried running the tutorial locally and I ran into some issues.

Bill Bejeck (bbejeck) · 2020-04-08T16:41:50Z

    compile "org.apache.avro:avro:1.8.2"
    implementation "org.slf4j:slf4j-simple:1.7.26"
-    implementation "org.apache.kafka:kafka-streams:2.3.0"
+    implementation "org.apache.kafka:kafka-streams:2.4.1"


With this change I get the following when trying to run the app at step 6

Exception in thread "main" java.lang.NoClassDefFoundError: org/apache/kafka/clients/admin/Admin at org.apache.kafka.streams.KafkaStreams.<init>(KafkaStreams.java:557) at io.confluent.developer.FindDistinctEvents.runRecipe(FindDistinctEvents.java:235) at io.confluent.developer.FindDistinctEvents.main(FindDistinctEvents.java:225) Caused by: java.lang.ClassNotFoundException: org.apache.kafka.clients.admin.Admin at java.net.URLClassLoader.findClass(URLClassLoader.java:382) at java.lang.ClassLoader.loadClass(ClassLoader.java:419) at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:352) at java.lang.ClassLoader.loadClass(ClassLoader.java:352) ... 3 more

Maybe just revert to 2.3.0 for now?

Yes, no time to investigate it right now... reverted to 2.3.0.

Bill Bejeck (bbejeck) · 2020-04-08T22:45:06Z

Thanks Ivan Ponomarev (@inponomarev) for the contribution. I've verified the tutorial steps and ran the test harness locally. I'm going to see about running the build for the PR before merging, but it will get merged soon regardless.

Thanks again!

use ValueTransformerWithKey instead of Transformer

8befd46

update the tutoral

57793a4

Bill Bejeck (bbejeck) requested changes Apr 7, 2020

View reviewed changes

Bill Bejeck (bbejeck) reviewed Apr 7, 2020

View reviewed changes

code review corrections

7db2a3b

Bill Bejeck (bbejeck) reviewed Apr 8, 2020

View reviewed changes

revert to 2.3.0

cd52f0f

Bill Bejeck (bbejeck) approved these changes Apr 8, 2020

View reviewed changes

Bill Bejeck (bbejeck) merged commit 7845c7a into confluentinc:master Apr 8, 2020

Uh oh!

Conversation

Ivan Ponomarev (inponomarev) commented Mar 19, 2020

Uh oh!

Ivan Ponomarev (inponomarev) commented Mar 19, 2020

Uh oh!

Bill Bejeck (bbejeck) commented Mar 26, 2020

Uh oh!

Ivan Ponomarev (inponomarev) commented Apr 7, 2020

Uh oh!

Bill Bejeck (bbejeck) left a comment

Choose a reason for hiding this comment

Uh oh!

Bill Bejeck (bbejeck) Apr 7, 2020 • 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!

Ivan Ponomarev (inponomarev) Apr 7, 2020

Choose a reason for hiding this comment

Uh oh!

Bill Bejeck (bbejeck) Apr 7, 2020

Choose a reason for hiding this comment

Uh oh!

Bill Bejeck (bbejeck) commented Apr 7, 2020

Uh oh!

Ivan Ponomarev (inponomarev) commented Apr 7, 2020

Uh oh!

Bill Bejeck (bbejeck) commented Apr 7, 2020

Uh oh!

Bill Bejeck (bbejeck) left a comment

Choose a reason for hiding this comment

Uh oh!

Bill Bejeck (bbejeck) Apr 8, 2020

Choose a reason for hiding this comment

Uh oh!

Ivan Ponomarev (inponomarev) Apr 8, 2020

Choose a reason for hiding this comment

Uh oh!

Bill Bejeck (bbejeck) commented Apr 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Bill Bejeck (bbejeck) Apr 7, 2020 •

edited

Loading