KAFKA-8319: Make KafkaStreamsTest a non-integration test class by guozhangwang · Pull Request #7382 · apache/kafka

guozhangwang · 2019-09-24T16:52:58Z

Previous KafkaStreamsTest takes 2min20s on my local laptop, because lots of its integration test which is producing / consuming records, and checking state directory file system takes lots of time. On the other hand, these tests should be well simplified with mocks.

This test reduces the test from a clumsy integration test class into a unit tests with mocks of its internal modules. And some other test functions should not be in KafkaStreamsTest actually and have been moved to other modular test classes. Now it takes 2s.

Also it helps removing the potential flakiness of the following (some of them are claimed resolved only because we have not seen them recently, but after looking at the test code I can verify they are still flaky):

KAFKA-5818 (the original JIRA ticket indeed exposed a real issue that has been fixed, but the test itself remains flaky)
KAFKA-6215
KAFKA-7921
KAFKA-7990
KAFKA-8319
KAFKA-8427

Committer Checklist (excluded from commit message)

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

guozhangwang · 2019-09-24T18:19:10Z

    @SuppressWarnings("deprecation")
    @Override
    public synchronized void close(long timeout, TimeUnit unit) {
-        ensureNotClosed();


This is a piggy-backed fix of MockConsumer: in the actual KafkaConsumer we do allow the close() call to be triggered multiple times, so there's no point checking this.

guozhangwang · 2019-09-24T18:19:28Z

    private final Admin adminClient;

-    private GlobalStreamThread globalStreamThread;
+    protected GlobalStreamThread globalStreamThread;


Similar to stream threads, making it accessible directly from KafkaStreamsTest.

package-private should also work, right?

guozhangwang · 2019-09-24T18:20:42Z

-    }
-
-    @Test
-    public void testOsDefaultSocketBufferSizes() {


These are moved to StreamsConfigTest.

guozhangwang · 2019-09-24T18:21:18Z

    @Override
    public Admin getAdmin(final Map<String, Object> config) {
-        return new MockAdminClient(cluster.nodes(), cluster.nodeById(0));
+        return new MockAdminClient(cluster.nodes(), cluster.nodeById(-1));


This is a piggy-backed fix: the controller's id is defined as -1, so with a single node cluster we should set it's controller as -1 not as 0.

guozhangwang · 2019-09-24T18:21:35Z


    private Cluster createCluster() {
-        final Node node = new Node(0, "localhost", 8121);
+        final Node node = new Node(-1, "localhost", 8121);


See the other comment below in MockClientSupplier.java

guozhangwang · 2019-09-24T18:22:40Z

-    }
-
-    @Test
-    public void shouldCleanupOldStateDirs() throws InterruptedException {


These are examples where we should really not rely on checking the underlying state directory files, but just check the mocked state-directory as the expected construction call, etc.

guozhangwang · 2019-09-24T18:23:25Z

-                                               final Collection<String> inputTopics,
-                                               final String outputTopic,
                                               final boolean shouldFilesExist) throws Exception {
-        final File baseDir = new File(TestUtils.IO_TMP_DIR + File.separator + "kafka-" + TestUtils.randomString(5));


See the other comment above: instead of checking the file system which is costly and flaky, we should just use a mock with expected constructors / function calls.

guozhangwang · 2019-09-24T18:23:42Z

-    }
+        PowerMock.replayAll();

-    public static class StateListenerStub implements KafkaStreams.StateListener {


Moved up in the file, no logical changes.

guozhangwang · 2019-09-24T18:24:37Z

-    }
-
-    @Test
-    public void testCannotStartTwice() {


This is a flaky test that should be removed: the streams.start() would only throw if we are still in the REBALANCING state, which is not really a good test anyways.

guozhangwang · 2019-09-24T18:25:32Z

    }

-    @Test
-    public void globalThreadShouldTimeoutWhenBrokerConnectionCannotBeEstablished() {


These two tests are not really for KafkaStreams but for StreamThreads / GlobalStreamThread.

Are these cases already covered by existing tests over there? I didn't see them moved as part of this PR.

The first test is covered in GlobalStreamThreadTest#shouldThrowStreamsExceptionOnStartupIfExceptionOccurred: we call partitionsFor during the initialization of the global task, which would throw if the broker is not connected.

The second test is covered in shouldNotThrowWhenPendingShutdownInRunOnce and shouldNotThrowWithoutPendingShutdownInRunOnce of StreamThreadTest to me.

sounds good! Thanks for the confirmation.

vvcephei

Hey @guozhangwang ,

Thanks so much for this. I made a quick pass (definitely didn't go over all the tests to make sure they are effectively the same), and everything looks reasonable. I left a few comments.

Thanks,
-John

vvcephei · 2019-09-24T19:40:29Z

    private final Admin adminClient;

-    private GlobalStreamThread globalStreamThread;
+    protected GlobalStreamThread globalStreamThread;


package-private should also work, right?

vvcephei · 2019-09-24T19:52:39Z

+        } finally {
+            streams.close();
+        }
+        streams.cleanUp();


What's up with this last one? Should it be in the finally block?

vvcephei · 2019-09-24T19:58:12Z

    }

-    @Test
-    public void globalThreadShouldTimeoutWhenBrokerConnectionCannotBeEstablished() {


Are these cases already covered by existing tests over there? I didn't see them moved as part of this PR.

…afkaStreamsTest-flaky

guozhangwang · 2019-09-25T00:47:24Z

-        props.setProperty(StreamsConfig.STATE_DIR_CONFIG, TestUtils.tempDirectory().getPath());
-        props.put(StreamsConfig.NUM_STREAM_THREADS_CONFIG, NUM_THREADS);
-
-        // make sure we have the global state thread running too


This comment is incorrect too :) it should be make sure we do NOT have global state thread.

vvcephei

Hey @guozhangwang ,

I made another, closer, pass. Everything looks good to me. I have a concern about one test, but my concern pre-dates this PR. I'll leave it to you whether you want to take the feedback or not. Regardless, your PR should already be a huge help.

Thanks,
-John

vvcephei · 2019-09-26T18:53:11Z


-        try {
+            th.start();
            th.join(30_000L);


Not sure I follow the logic of this test... We want to show that it doesn't block, but then we just assert that the call doesn't take longer than 30 seconds.

It seems like this test is sensitive to timing, which is not ideal. I guess that what we really wanted to test is that the close method never calls Thread.sleep()/Thread.join() or maybe just never calls a specific list of blocking methods on the clients. Now that we have all the mocks above, it seems like we can make this assertion directly.

I agree, I've actually simplify the logic to just check that close() call returns false, since with mock-time it would never elapse any time, and if the call ever waits on the state transition it would not return ever.

bbejeck

Thanks for this PR @guozhangwang, this will really help with the length of time tests take. I just have one minor comment otherwise this LGTM.

bbejeck · 2019-09-26T23:34:18Z

 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;

 @Category({IntegrationTest.class})


Do we still want this labeled as an integration test?

cadonna · 2019-09-26T11:00:46Z

 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;

 @Category({IntegrationTest.class})


If it is no more an integration test, this should be removed.

cadonna · 2019-09-26T11:04:29Z


 @Category({IntegrationTest.class})
+@RunWith(PowerMockRunner.class)
+@PrepareForTest({KafkaStreams.class, StreamThread.class})


Why do you need to prepare KafkaStreams for testing? Only classes that are mocked and that are either final or have static methods need to be prepared.

We actually need to prepare KafkaStreams for cases like metrics reporter registration.

…afkaStreamsTest-flaky

Conflicts: * .gitignore: addition of clients/src/generated-test was near local additions for support-metrics. * checkstyle/suppressions.xml: upstream refactoring of exclusions for generator were near the local changes for support-metrics. * gradle.properties: scala version bump caused a minor conflict due to the kafka version change locally. gradle/dependencies.gradle: bcpkix version bump was near avro additions in the local version. * apache-github/trunk: (49 commits) KAFKA-8471: Replace control requests/responses with automated protocol (apache#7353) MINOR: Don't generate unnecessary strings for debug logging in FetchSessionHandler (apache#7394) MINOR:fixed typo and removed outdated varilable name (apache#7402) KAFKA-8934: Create version file during build for Streams (apache#7397) KAFKA-8319: Make KafkaStreamsTest a non-integration test class (apache#7382) KAFKA-6883: Add toUpperCase support to sasl.kerberos.principal.to.local rule (KIP-309) KAFKA-8907; Return topic configs in CreateTopics response (KIP-525) (apache#7380) MINOR: Address review comments for KIP-504 authorizer changes (apache#7379) MINOR: add versioning to request and response headers (apache#7372) KAFKA-7273: Extend Connect Converter to support headers (apache#6362) MINOR: improve the Kafka RPC code generator (apache#7340) MINOR: Improve the org.apache.kafka.common.protocol code (apache#7344) KAFKA-8880: Docs on upgrade-guide (apache#7385) KAFKA-8179: do not suspend standby tasks during rebalance (apache#7321) KAFKA-8580: Compute RocksDB metrics (apache#7263) KAFKA-8880: Add overloaded function of Consumer.committed (apache#7304) HOTFIX: fix Kafka Streams upgrade note for broker backward compatibility (apache#7363) KAFKA-8848; Update system tests to use new AclAuthorizer (apache#7374) MINOR: remove unnecessary null check (apache#7299) KAFKA-6958: Overload methods for group and windowed stream to allow to name operation name using the new Named class (apache#6413) ...

…e#7382) Previous KafkaStreamsTest takes 2min20s on my local laptop, because lots of its integration test which is producing / consuming records, and checking state directory file system takes lots of time. On the other hand, these tests should be well simplified with mocks. This test reduces the test from a clumsy integration test class into a unit tests with mocks of its internal modules. And some other test functions should not be in KafkaStreamsTest actually and have been moved to other modular test classes. Now it takes 2s. Also it helps removing the potential flakiness of the following (some of them are claimed resolved only because we have not seen them recently, but after looking at the test code I can verify they are still flaky): * KAFKA-5818 (the original JIRA ticket indeed exposed a real issue that has been fixed, but the test itself remains flaky) * KAFKA-6215 * KAFKA-7921 * KAFKA-7990 * KAFKA-8319 * KAFKA-8427 Reviewers: Bill Bejeck <bill@confluent.io>, John Roesler <john@confluent.io>, Bruno Cadonna <bruno@confluent.io> This commit was cherry-picked from trunk and adapted.

#8352) Previous KafkaStreamsTest takes 2min20s on my local laptop, because lots of its integration test which is producing / consuming records, and checking state directory file system takes lots of time. On the other hand, these tests should be well simplified with mocks. This test reduces the test from a clumsy integration test class into a unit tests with mocks of its internal modules. And some other test functions should not be in KafkaStreamsTest actually and have been moved to other modular test classes. Now it takes 2s. Also it helps removing the potential flakiness of the following (some of them are claimed resolved only because we have not seen them recently, but after looking at the test code I can verify they are still flaky): * KAFKA-5818 (the original JIRA ticket indeed exposed a real issue that has been fixed, but the test itself remains flaky) * KAFKA-6215 * KAFKA-7921 * KAFKA-7990 * KAFKA-8319 * KAFKA-8427 Reviewers: Bill Bejeck <bill@confluent.io>, John Roesler <john@confluent.io>, Bruno Cadonna <bruno@confluent.io> This commit was cherry-picked from trunk and adapted. Co-authored-by: Guozhang Wang <wangguoz@gmail.com> Reviewers: Guozhang Wang <wangguoz@gmail.com>

guozhangwang added 3 commits September 23, 2019 23:24

half way done

e395306

one test left

5f3bd10

checkstyle fix

7fc7d38

guozhangwang changed the title ~~MINOR [WIP]: Fix KafkaStreamsTest~~ MINOR: Fix KafkaStreamsTest Sep 24, 2019

guozhangwang changed the title ~~MINOR: Fix KafkaStreamsTest~~ KAFKA-8319: Make KafkaStreamsTest a non-integration test class Sep 24, 2019

guozhangwang commented Sep 24, 2019

View reviewed changes

mjsax added the streams label Sep 24, 2019

vvcephei reviewed Sep 24, 2019

View reviewed changes

guozhangwang added 2 commits September 24, 2019 16:18

Merge branch 'trunk' of https://github.com/apache/kafka into KMinor-K…

ef3fdb1

…afkaStreamsTest-flaky

github comments

333c8b1

guozhangwang commented Sep 25, 2019

View reviewed changes

vvcephei approved these changes Sep 26, 2019

View reviewed changes

bbejeck approved these changes Sep 26, 2019

View reviewed changes

cadonna reviewed Sep 27, 2019

View reviewed changes

guozhangwang added 2 commits September 27, 2019 11:06

Merge branch 'trunk' of https://github.com/apache/kafka into KMinor-K…

7d49e32

…afkaStreamsTest-flaky

github comments

5f40f55

guozhangwang merged commit 22434e6 into apache:trunk Sep 27, 2019

guozhangwang deleted the KMinor-KafkaStreamsTest-flaky branch April 24, 2020 23:52

Conversation

guozhangwang commented Sep 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvcephei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvcephei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbejeck left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

guozhangwang commented Sep 24, 2019 •

edited

Loading