vvcephei commented on a change in pull request #9414:
URL: https://github.com/apache/kafka/pull/9414#discussion_r504054028
##########
File path:
streams/src/test/java/org/apache/kafka/streams/processor/internals/StateDirectoryTest.java
##########
@@ -602,6 +606,11 @@ public void shouldLogStateDirCleanerMessage() {
directory.cleanRemovedTasks(cleanupDelayMs);
assertThat(appender.getMessages(), hasItem(endsWith("ms has
elapsed (cleanup delay is " + cleanupDelayMs + "ms).")));
}
+
+ // if appDir is empty, it is deleted in process.
+ // since we did not call StateDirectory#clean, the global state
directory is not deleted and appDir also.
+ assertTrue(appDir.exists());
+ assertArrayEquals(appDir.list(), new String[]{"0_0"});
Review comment:
We shouldn't add unrelated checks to tests in general, it just makes the
tests more confusing. If you want to verify that the directory still exists
after closing when we don't call `clean()`, we should just add a new test for
it.
##########
File path:
streams/src/test/java/org/apache/kafka/streams/processor/internals/StateDirectoryTest.java
##########
@@ -510,8 +511,8 @@ public void
shouldCleanupAllTaskDirectoriesIncludingGlobalOne() {
directory.clean();
- assertEquals(Collections.emptySet(), Arrays.stream(
-
Objects.requireNonNull(appDir.listFiles())).collect(Collectors.toSet()));
+ // if appDir is empty, it is deleted in StateDirectory#clean process.
+ assertTrue(!appDir.exists());
Review comment:
```suggestion
assertFalse(appDir.exists());
```
:)
##########
File path:
streams/src/main/java/org/apache/kafka/streams/processor/internals/StateDirectory.java
##########
@@ -301,6 +301,22 @@ public synchronized void clean() {
);
throw new StreamsException(exception);
}
+
+ try {
+ if (hasPersistentStores && stateDir.exists() &&
!stateDir.delete()) {
Review comment:
```suggestion
if (stateDir.exists() && !stateDir.delete()) {
```
Do we need to check `hasPersistentStores` here? It seems sufficient just to
check if the directory exists.
##########
File path:
streams/src/test/java/org/apache/kafka/streams/processor/internals/StateDirectoryTest.java
##########
@@ -586,6 +587,9 @@ public void shouldLogManualUserCallMessage() {
hasItem(endsWith("as user calling cleanup."))
);
}
+
+ // if appDir is empty, it is deleted in StateDirectory#clean process.
+ assertTrue(!appDir.exists());
Review comment:
Similar feedback here as below. This test is about logging, not cleanup.
You've already added a check to the "shouldCleanup" test, so we don't need one
here.
----------------------------------------------------------------
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:
us...@infra.apache.org
