[GitHub] [kafka] guozhangwang commented on a change in pull request #8578: KAFKA-9875: Make integration tests more resilient

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

guozhangwang commented on a change in pull request #8578:
URL: https://github.com/apache/kafka/pull/8578#discussion_r417015562



##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/OptimizedKTableIntegrationTest.java
##########
@@ -215,13 +216,13 @@ public void onRestoreEnd(final TopicPartition topicPartition, final String store
     }
 
     private Properties streamsConfiguration() {
-        final String applicationId = "streamsApp";
+        final String safeTestName = safeUniqueTestName(getClass(), testName);
         final Properties config = new Properties();
         config.put(StreamsConfig.TOPOLOGY_OPTIMIZATION, StreamsConfig.OPTIMIZE);
-        config.put(StreamsConfig.APPLICATION_ID_CONFIG, applicationId + name.getMethodName());
+        config.put(StreamsConfig.APPLICATION_ID_CONFIG, "app-" + safeTestName);
         config.put(StreamsConfig.APPLICATION_SERVER_CONFIG, "localhost:" + (++port));
         config.put(StreamsConfig.BOOTSTRAP_SERVERS_CONFIG, cluster.bootstrapServers());
-        config.put(StreamsConfig.STATE_DIR_CONFIG, TestUtils.tempDirectory(applicationId).getPath());
+        config.put(StreamsConfig.STATE_DIR_CONFIG, TestUtils.tempDirectory().getPath());

Review comment:
       Is it safer to encode the appID as part of the dir path to avoid collision?

##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/utils/IntegrationTestUtils.java
##########
@@ -145,12 +162,12 @@ public static void cleanStateBeforeTest(final EmbeddedKafkaCluster cluster, fina
         }
     }
 
-    public static void cleanStateAfterTest(final EmbeddedKafkaCluster cluster, final KafkaStreams driver) {
-        driver.cleanUp();
+    public static void quietlyCleanStateAfterTest(final EmbeddedKafkaCluster cluster, final KafkaStreams driver) {
         try {
+            driver.cleanUp();
             cluster.deleteAllTopicsAndWait(DEFAULT_TIMEOUT);
-        } catch (final InterruptedException e) {
-            throw new RuntimeException(e);
+        } catch (final RuntimeException | InterruptedException e) {
+            LOG.warn("Ignoring failure to clean test state", e);
         }

Review comment:
       req: Actually deleting topics after test is critical for some tests: I've encountered some cases where the same topics are reused mistakenly across different test cases within the single class. But I feel that it is better to put the topic deletion in the `@after` function while leaving `cleanUp()` as part of the test function itself.

##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/ResetPartitionTimeIntegrationTest.java
##########
@@ -156,7 +157,7 @@ public void shouldPreservePartitionTimeOnKafkaStreamRestart() {
             assertThat(lastRecordedTimestamp, is(5000L));
         } finally {
             kafkaStreams.close();
-            cleanStateAfterTest(CLUSTER, kafkaStreams);
+            quietlyCleanStateAfterTest(CLUSTER, kafkaStreams);

Review comment:
       nit: we can put kafkaStreams in a try block.

##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/LagFetchIntegrationTest.java
##########
@@ -106,7 +108,7 @@ public void before() {
 
         consumerConfiguration = new Properties();
         consumerConfiguration.setProperty(ConsumerConfig.BOOTSTRAP_SERVERS_CONFIG, CLUSTER.bootstrapServers());
-        consumerConfiguration.setProperty(ConsumerConfig.GROUP_ID_CONFIG, name.getMethodName() + "-consumer");
+        consumerConfiguration.setProperty(ConsumerConfig.GROUP_ID_CONFIG, safeTestName + "-consumer");

Review comment:
       Somewhere else it is set as `"group-" + safeTestName`, is this change intentional?

##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/SuppressionDurabilityIntegrationTest.java
##########
@@ -243,7 +251,7 @@ public void shouldRecoverBufferAfterShutdown() {
 
         } finally {
             driver.close();
-            cleanStateAfterTest(CLUSTER, driver);
+            quietlyCleanStateAfterTest(CLUSTER, driver);

Review comment:
       nit: ditto here, we can put `driver` in the try block. And ditto elsewhere.




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