[GitHub] [kafka] mjsax commented on a change in pull request #9733: KAFKA-10017: fix 2 issues in EosBetaUpgradeIntegrationTest

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

mjsax commented on a change in pull request #9733:
URL: https://github.com/apache/kafka/pull/9733#discussion_r543863413



##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/EosBetaUpgradeIntegrationTest.java
##########
@@ -938,11 +943,13 @@ public void close() {}
 
         final KafkaStreams streams = new KafkaStreams(builder.build(), config, new TestKafkaClientSupplier());
         streams.setUncaughtExceptionHandler((t, e) -> {
-            if (uncaughtException != null) {
+            // should only have our injected exception or commit exception
+            if (!(e instanceof RuntimeException) && !(e.getMessage().contains("test exception"))) {
+                // The exception won't cause the test fail since we actually "expected" exception thrown and failed the stream.
+                // So, log to stderr for debugging when the exception is not what we expected
                 e.printStackTrace(System.err);
                 fail("Should only get one uncaught exception from Streams.");

Review comment:
       Seems this `fail` did not work as expected? Otherwise the test would have failed all the time? Maybe we should rather set a boolean flag that we evaluate outside of the callback to let the test fail?
   
   Also, we one run with zero exceptions and one run with 2 exception (one exception type each) -- not 4.
   
   

##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/EosBetaUpgradeIntegrationTest.java
##########
@@ -256,8 +261,8 @@ public void shouldUpgradeFromEosAlphaToEosBeta() throws Exception {
             streams2Alpha.cleanUp();
             streams2Alpha.start();
             assignmentListener.waitForNextStableAssignment(MAX_WAIT_TIME_MS);

Review comment:
       To avoid the issue you point out, we actually use this line. As a matter of fact, there is no guarantee if there would be even more than two rebalances. \cc @ableegoldman might be able to provide more input?




----------------------------------------------------------------
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.

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