HOTFIX: fix flaky StateDirectoryTest.shouldReturnEmptyArrayIfListFile…

[chia7712]

StateDirectoryTest.shouldReturnEmptyArrayIfListFilesReturnsNull always moves the stage dir to /tmp/state-renamed so it always fails if there is already a folder (for example, the stuff leaved by previous test)

We should just delete the folder to make stage dir disappear

related to #8304

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[abbccdda]

LGTM. @ableegoldman Could you take a look?

[ableegoldman]

We don't want to remove the stateDir entirely, because then stateDir.exists will return false and we won't actually call listFiles which is what this test is trying to test. Can we just rename to "state-renamed" + TestUtils.randomString(5)? Or, clean up the previous contents at the beginning of the test?

That said, cleanUp should be called after every test, so there shouldn't be any left over state...right? cc/ @guozhangwang

[guozhangwang]

That's true, but I suspect that this leftover folder is not from this test suite but likely from others? I think the right fix here should be to check which test (highly doubt it is one of the StateDirectoryTest because of cleanup ) has this leftover and fix that one instead.

[ableegoldman]

Hm...we actually create a new stateDir with 5 random characters so that seems pretty unlikely.

@chia7712 For now we can just add a + TestUtils.randomString(5) to the renamed name to fix the flakyness. In fact we can actually remove everything before stateDir.rename in this test, it's identical to what's done in @Before. I think it's leftover from a different approach to writing this test.

[chia7712]

@guozhangwang @ableegoldman thanks for feedback! I understand the purpose of this test case now.

That said, cleanUp should be called after every test, so there shouldn't be any left over state...right?
That's true, but I suspect that this leftover folder is not from this test suite but likely from others?

not really. java File is an immutable object so the renameTo does not change the file point. (see https://docs.oracle.com/javase/7/docs/api/java/io/File.html?is-external=true).

We don't want to remove the stateDir entirely, because then stateDir.exists will return false and we won't actually call listFiles which is what this test is trying to test.

As file is immutable, stateDir.exists returns false after renaming.

It seems to me the solution is to remove all files in StateDirectory.stageDir instead of renaming. It fix following issues.

1. the stage dir is not deleted by after()
2. stateDir.exists return false as origin file is existent

[guozhangwang]

@chia7712 @ableegoldman I think about this case a bit more, and I think maybe we can refactor this test by creating the stateDir as a file and not a directory, so that stateDir.exists would return true while listFiles would return null. WDYT?

[chia7712]

ok. I was confused by the inconsistent naming. the folder in StateDirectory should be called appDir rather than stateDir

    public StateDirectory(final StreamsConfig config, final Time time, final boolean hasPersistentStores) {
        final String stateDirName = config.getString(StreamsConfig.STATE_DIR_CONFIG);
        final File baseDir = new File(stateDirName);
         ...
        stateDir = new File(baseDir, appId);
        ...
    }

Correct my previous comments :(

the targets I have to address are shown below.

1. make sure the stateDir in StateDirectory points to nothing in order to make listFiles return null.
2. make sure the stateDir in test case is deleted by after

[chia7712]

I think about this case a bit more, and I think maybe we can refactor this test by creating the stateDir as a file and not a directory, so that stateDir.exists would return true while listFiles would return null. WDYT?

yep. that is what I have done :)

[guozhangwang]

LGTM!

[guozhangwang]

Thanks for the quick fix @chia7712 !! Merging to trunk now.

[guozhangwang]

The latest build seems still flaky but on a different test case:

Error
java.lang.AssertionError: expected:<[]> but was:<[/tmp/kafka-M1qSA/applicationId/0_0]>
Stacktrace
java.lang.AssertionError: expected:<[]> but was:<[/tmp/kafka-M1qSA/applicationId/0_0]>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:120)
	at org.junit.Assert.assertEquals(Assert.java:146)
	at org.apache.kafka.streams.processor.internals.StateDirectoryTest.shouldOnlyListNonEmptyTaskDirectories(StateDirectoryTest.java:358)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)

I'll take a look into this.

[guozhangwang]

I've pushed a HOTFIX commit to address this issue, cc @mjsax @ableegoldman

[chia7712]

I've pushed a HOTFIX commit to address this issue

c0cff61