[GitHub] [kafka] cadonna commented on a change in pull request #10958: MINOR: Add default serde in stream test to fix QA ERROR

[GitBox <gi...@apache.org>]

cadonna commented on a change in pull request #10958:
URL: https://github.com/apache/kafka/pull/10958#discussion_r662865972



##########
File path: streams/test-utils/src/test/java/org/apache/kafka/streams/TopologyTestDriverTest.java
##########
@@ -98,6 +98,8 @@
                 mkEntry(StreamsConfig.STATE_DIR_CONFIG, TestUtils.tempDirectory().getAbsolutePath())
         ));
         config.putAll(overrides);
+        config.putIfAbsent(StreamsConfig.DEFAULT_KEY_SERDE_CLASS_CONFIG, Serdes.ByteArraySerde.class);
+        config.putIfAbsent(StreamsConfig.DEFAULT_VALUE_SERDE_CLASS_CONFIG, Serdes.ByteArraySerde.class);

Review comment:
       Could you put these config settings before the overrides on line 100? Would be not so good to silently add the default serdes if the the overrides possibly unset them. I would also just use `put()` instead of `putIfAbsent()`.

##########
File path: streams/test-utils/src/main/java/org/apache/kafka/streams/TopologyTestDriver.java
##########
@@ -315,6 +316,9 @@ private TopologyTestDriver(final InternalTopologyBuilder builder,
         configCopy.putIfAbsent(StreamsConfig.BOOTSTRAP_SERVERS_CONFIG, "dummy-bootstrap-host:0");
         // provide randomized dummy app-id if it's not specified
         configCopy.putIfAbsent(StreamsConfig.APPLICATION_ID_CONFIG,  "dummy-topology-test-driver-app-id-" + ThreadLocalRandom.current().nextInt());
+        // provide default serde since we change default serde to be null in KIP-741
+        configCopy.putIfAbsent(StreamsConfig.DEFAULT_KEY_SERDE_CLASS_CONFIG, Serdes.ByteArraySerde.class);
+        configCopy.putIfAbsent(StreamsConfig.DEFAULT_VALUE_SERDE_CLASS_CONFIG, Serdes.ByteArraySerde.class);

Review comment:
       Since the `TopologyTestDriver` should test a topology and somehow the default serdes are part of the topology under test, the `TopologyTestDriver` should fail, if no default serdes are provided by the users but needed. Hence, I think, it is not a good idea to silently add the default serdes here. 
   That means, you have to provide deserializers and serializers in some tests in `TopologyTestDriverTest`. For that look for methods called `setup*()` (e.g. `setupSourceSinkTopology()`) and add the deserializers and serializers there. In some tests, you need to add them directly in the test.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org