[GitHub] [kafka] vvcephei commented on a change in pull request #9414: KAFKA-10585: Kafka Streams should clean up the state store directory from cleanup

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

vvcephei commented on a change in pull request #9414:
URL: https://github.com/apache/kafka/pull/9414#discussion_r524437121



##########
File path: streams/src/test/java/org/apache/kafka/streams/processor/internals/StateDirectoryTest.java
##########
@@ -186,19 +186,29 @@ public void shouldReportDirectoryEmpty() throws IOException {
 
     @Test
     public void shouldThrowProcessorStateException() throws IOException {

Review comment:
       Since you have modified the purpose of this test, maybe we can go ahead and give the test a more specific name as well. 
   
   ```suggestion
       public void shouldThrowProcessorStateExceptionIfTaskDirectoryIsOccupiedByFile() throws IOException {
   ```
   
   Also, I won't dispute the value of checking this condition, but would like to point out that this test was previously verifying a specific error on failure to create the task directory, and now we are no longer checking that failure. In other words, we were previously verifying "task directory [%s] doesn't exist and couldn't be created", but now we are only verifying the separate and specific failure reason "task directory path [%s] is already occupied".
   
   It actually seems like maybe we don't need to check that specific `!taskDir.isDirectory()` case, since it seems like having this file sitting there should cause a failure to create the task directory, right?

##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/EOSUncleanShutdownIntegrationTest.java
##########
@@ -140,9 +140,6 @@ public void shouldWorkWithUncleanShutdownWipeOutStateStore() throws InterruptedE
             IntegrationTestUtils.produceSynchronously(producerConfig, false, input, Optional.empty(),
                 singletonList(new KeyValueTimestamp<>("k1", "v1", 0L)));
 
-            TestUtils.waitForCondition(stateDir::exists,
-                "Failed awaiting CreateTopics first request failure");

Review comment:
       Can you explain why we need to remove this? It seems like the application must have created the state directory by this point, right?




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