You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/11/20 07:45:01 UTC
[GitHub] [kafka] showuon opened a new pull request #9629: KAFKA-10754: fix flaky tests by waiting kafka streams be in running state before assert
showuon opened a new pull request #9629:
URL: https://github.com/apache/kafka/pull/9629
The flaky test is because we didn't wait for the streams become RUNNING before verifying the state becoming ERROR state. This fix explicitly wait for the streams become RUNNING state. Also, I found we didn't put the 2nd stream into try resource block, so it won't close the stream after tests.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
----------------------------------------------------------------
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
[GitHub] [kafka] ableegoldman merged pull request #9629: KAFKA-10754: fix flaky tests by waiting kafka streams be in running state before assert
Posted by GitBox <gi...@apache.org>.
ableegoldman merged pull request #9629:
URL: https://github.com/apache/kafka/pull/9629
----------------------------------------------------------------
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
[GitHub] [kafka] showuon commented on a change in pull request #9629: KAFKA-10754: fix flaky tests by waiting kafka streams be in running state before assert
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #9629:
URL: https://github.com/apache/kafka/pull/9629#discussion_r527741948
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/StreamsUncaughtExceptionHandlerIntegrationTest.java
##########
@@ -110,83 +110,45 @@ public void teardown() throws IOException {
}
@Test
- public void shouldShutdownThreadUsingOldHandler() throws Exception {
+ public void shouldShutdownThreadUsingOldHandler() throws InterruptedException {
try (final KafkaStreams kafkaStreams = new KafkaStreams(builder.build(), properties)) {
- final CountDownLatch latch = new CountDownLatch(1);
final AtomicBoolean flag = new AtomicBoolean(false);
kafkaStreams.setUncaughtExceptionHandler((t, e) -> flag.set(true));
StreamsTestUtils.startKafkaStreamsAndWaitForRunningState(kafkaStreams);
-
produceMessages(0L, inputTopic, "A");
- waitForApplicationState(Collections.singletonList(kafkaStreams), KafkaStreams.State.ERROR, Duration.ofSeconds(15));
TestUtils.waitForCondition(flag::get, "Handler was called");
+ waitForApplicationState(Collections.singletonList(kafkaStreams), KafkaStreams.State.ERROR, DEFAULT_DURATION);
Review comment:
We should wait for the `uncaughtExceptionHandler` got called before waiting for the streams state change.
----------------------------------------------------------------
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
[GitHub] [kafka] showuon edited a comment on pull request #9629: KAFKA-10754: fix flaky tests by waiting kafka streams be in running state before assert
Posted by GitBox <gi...@apache.org>.
showuon edited a comment on pull request #9629:
URL: https://github.com/apache/kafka/pull/9629#issuecomment-731557695
All tests passed, Yeah!
----------------------------------------------------------------
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
[GitHub] [kafka] showuon commented on a change in pull request #9629: KAFKA-10754: fix flaky tests by waiting kafka streams be in running state before assert
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #9629:
URL: https://github.com/apache/kafka/pull/9629#discussion_r527495475
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/StreamsUncaughtExceptionHandlerIntegrationTest.java
##########
@@ -145,48 +142,12 @@ public void shouldShutdownClient() throws Exception {
@Test
public void shouldShutdownApplication() throws Exception {
- final Topology topology = builder.build();
-
- try (final KafkaStreams kafkaStreams = new KafkaStreams(topology, properties)) {
- final KafkaStreams kafkaStreams1 = new KafkaStreams(topology, properties);
Review comment:
forgot to put the kafkaStreams1 into the try resource block 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:
users@infra.apache.org
[GitHub] [kafka] showuon edited a comment on pull request #9629: KAFKA-10754: fix flaky tests by waiting kafka streams be in running state before assert
Posted by GitBox <gi...@apache.org>.
showuon edited a comment on pull request #9629:
URL: https://github.com/apache/kafka/pull/9629#issuecomment-731520316
@ableegoldman , thanks for pointing it out. After investigation, I found the test `StreamsUncaughtExceptionHandlerIntegrationTest.shouldShutdownThreadUsingOldHandler` failed is because we set 2 stream threads for this test. So when we got the `uncaughtException`, we shutdown the thread, and **rebalancing** to the other thread. And we have to wait for rebalancing completes, and later another exception thrown in the other thread, then the stream will turn into `ERROR` state, which is why it is so flaky.
I default set to 1 stream thread in this test since other tests will set to the expected thread number before testing.
The fix is in this commit: https://github.com/apache/kafka/pull/9629/commits/75e2d261aff819ed8c7a1ec154d64a8e2e1c626e. Thank you.
----------------------------------------------------------------
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
[GitHub] [kafka] showuon edited a comment on pull request #9629: KAFKA-10754: fix flaky tests by waiting kafka streams be in running state before assert
Posted by GitBox <gi...@apache.org>.
showuon edited a comment on pull request #9629:
URL: https://github.com/apache/kafka/pull/9629#issuecomment-731520316
@ableegoldman , thanks for pointing it out. After investigation, I found the test `StreamsUncaughtExceptionHandlerIntegrationTest.shouldShutdownThreadUsingOldHandler` failed is because we set 2 stream threads for this test. So when we got the `uncaughtException`, we shutdown the thread, and **rebalancing** to the other thread. And we have to wait for rebalancing completes, and later another exception thrown in the thread, then the stream will turn into `ERROR` state, which is why it is so flaky.
I default set to 1 stream thread in this test since other tests will set to the expected thread number before testing.
The fix is in this commit: https://github.com/apache/kafka/pull/9629/commits/e6d39f6dc15a198a5a58d34d239a1021eeaf43b7. Thank you.
----------------------------------------------------------------
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
[GitHub] [kafka] wcarlson5 commented on pull request #9629: KAFKA-10754: fix flaky tests by waiting kafka streams be in running state before assert
Posted by GitBox <gi...@apache.org>.
wcarlson5 commented on pull request #9629:
URL: https://github.com/apache/kafka/pull/9629#issuecomment-733088175
These changes LGTM. WDYT @ableegoldman? thanks for the PR!
----------------------------------------------------------------
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
[GitHub] [kafka] showuon edited a comment on pull request #9629: KAFKA-10754: fix flaky tests by waiting kafka streams be in running state before assert
Posted by GitBox <gi...@apache.org>.
showuon edited a comment on pull request #9629:
URL: https://github.com/apache/kafka/pull/9629#issuecomment-732646473
@ableegoldman @wcarlson5 , thanks for the comments. Now I know why it sets the default treads to 2. So, to make the test more reliable, I'll do:
1. wait for the state become running (as before)
2. wait for the 1st time handler got called in current thread
3. wait for the 2nd time handler got called after rebalancing
4. wait for the stream state turned into ERROR state (as before)
This should make this test more reliable. What do you think?
commit: https://github.com/apache/kafka/pull/9629/commits/2b6d0a2d285b5b7fc0a9a8474712870f6f7a767e
----------------------------------------------------------------
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
[GitHub] [kafka] showuon commented on pull request #9629: KAFKA-10754: fix flaky tests by waiting kafka streams be in running state before assert
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #9629:
URL: https://github.com/apache/kafka/pull/9629#issuecomment-731557695
All tests passed
----------------------------------------------------------------
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
[GitHub] [kafka] showuon commented on a change in pull request #9629: KAFKA-10754: fix flaky tests by waiting kafka streams be in running state before assert
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #9629:
URL: https://github.com/apache/kafka/pull/9629#discussion_r529182580
##########
File path: streams/src/main/java/org/apache/kafka/streams/KafkaStreams.java
##########
@@ -379,13 +379,12 @@ public void setUncaughtExceptionHandler(final Thread.UncaughtExceptionHandler un
}
/**
- * Set the handler invoked when an {@link StreamsConfig#NUM_STREAM_THREADS_CONFIG internal thread}
+ * Set the handler invoked when an {@link StreamsConfig#NUM_STREAM_THREADS_CONFIG} internal thread
Review comment:
Updated. Thanks.
----------------------------------------------------------------
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
[GitHub] [kafka] showuon commented on a change in pull request #9629: KAFKA-10754: fix flaky tests by waiting kafka streams be in running state before assert
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #9629:
URL: https://github.com/apache/kafka/pull/9629#discussion_r527720582
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/utils/IntegrationTestUtils.java
##########
@@ -903,16 +903,19 @@ public static void startApplicationAndWaitUntilRunning(final List<KafkaStreams>
}
/**
- * Waits for the given {@link KafkaStreams} instances to all be in a {@link State#RUNNING}
- * state. Prefer {@link #startApplicationAndWaitUntilRunning(List, Duration)} when possible
+ * Waits for the given {@link KafkaStreams} instances to all be in a specific {@link State}.
+ * Prefer {@link #startApplicationAndWaitUntilRunning(List, Duration)} when possible
* because this method uses polling, which can be more error prone and slightly slower.
*
* @param streamsList the list of streams instances to run.
- * @param timeout the time to wait for the streams to all be in {@link State#RUNNING} state.
+ * @param state the expected state that all the streams to be in within timeout
+ * @param timeout the time to wait for the streams to all be in the specific state.
+ *
+ * @throws InterruptedException if the streams doesn't change to the expected state in time.
*/
public static void waitForApplicationState(final List<KafkaStreams> streamsList,
final State state,
- final Duration timeout) throws Exception {
+ final Duration timeout) throws InterruptedException {
Review comment:
We should throw a specific kind of exception, not an `Exception`.
----------------------------------------------------------------
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
[GitHub] [kafka] ableegoldman commented on pull request #9629: KAFKA-10754: fix flaky tests by waiting kafka streams be in running state before assert
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on pull request #9629:
URL: https://github.com/apache/kafka/pull/9629#issuecomment-731359086
Hey @showuon thanks for the quick fix! I notice that `StreamsUncaughtExceptionHandlerIntegrationTest.shouldShutdownThreadUsingOldHandler` still failed on the JDK15 PR build, can you look into that?
----------------------------------------------------------------
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
[GitHub] [kafka] showuon commented on pull request #9629: KAFKA-10754: fix flaky tests by waiting kafka streams be in running state before assert
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #9629:
URL: https://github.com/apache/kafka/pull/9629#issuecomment-730972399
@ableegoldman @wcarlson5 , could you help review this PR? Thanks.
----------------------------------------------------------------
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
[GitHub] [kafka] showuon edited a comment on pull request #9629: KAFKA-10754: fix flaky tests by waiting kafka streams be in running state before assert
Posted by GitBox <gi...@apache.org>.
showuon edited a comment on pull request #9629:
URL: https://github.com/apache/kafka/pull/9629#issuecomment-731520316
@ableegoldman , thanks for pointing it out. After investigation, I found the test `StreamsUncaughtExceptionHandlerIntegrationTest.shouldShutdownThreadUsingOldHandler` failed is because we set 2 stream threads for this test. So when we got the `uncaughtException`, we shutdown the thread, and **rebalancing** to the other thread. And we have to wait for rebalancing completes, and later another exception thrown in the thread, then the stream will turn into `ERROR` state, which is why it is so flaky.
I default set to 1 stream thread in this test since other tests will set to the expected thread number before testing.
The fix is in this commit: https://github.com/apache/kafka/pull/9629/commits/75e2d261aff819ed8c7a1ec154d64a8e2e1c626e. Thank you.
----------------------------------------------------------------
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
[GitHub] [kafka] showuon commented on pull request #9629: KAFKA-10754: fix flaky tests by waiting kafka streams be in running state before assert
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #9629:
URL: https://github.com/apache/kafka/pull/9629#issuecomment-731520316
@ableegoldman , thanks for pointing it out. After investigation, I found the test `StreamsUncaughtExceptionHandlerIntegrationTest.shouldShutdownThreadUsingOldHandler` failed is because we set 2 stream threads for this test. So when we got the `uncaughtException`, we shutdown the thread, and **rebalancing** to the other thread. And we have to wait for another exception thrown in the other thread, then the stream will turn into `ERROR` state, which is why it is so flaky.
I default set to 1 stream thread in this test since other tests will set to the expected thread number before testing.
The fix is in this commit: https://github.com/apache/kafka/pull/9629/commits/75e2d261aff819ed8c7a1ec154d64a8e2e1c626e. Thank you.
----------------------------------------------------------------
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
[GitHub] [kafka] showuon commented on a change in pull request #9629: KAFKA-10754: fix flaky tests by waiting kafka streams be in running state before assert
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #9629:
URL: https://github.com/apache/kafka/pull/9629#discussion_r527495985
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/StreamsUncaughtExceptionHandlerIntegrationTest.java
##########
@@ -145,48 +142,12 @@ public void shouldShutdownClient() throws Exception {
@Test
public void shouldShutdownApplication() throws Exception {
- final Topology topology = builder.build();
-
- try (final KafkaStreams kafkaStreams = new KafkaStreams(topology, properties)) {
- final KafkaStreams kafkaStreams1 = new KafkaStreams(topology, properties);
- final CountDownLatch latch = new CountDownLatch(1);
- kafkaStreams.setUncaughtExceptionHandler((t, e) -> fail("should not hit old handler"));
- kafkaStreams1.setUncaughtExceptionHandler((t, e) -> fail("should not hit old handler"));
- kafkaStreams.setUncaughtExceptionHandler(exception -> SHUTDOWN_APPLICATION);
- kafkaStreams1.setUncaughtExceptionHandler(exception -> SHUTDOWN_APPLICATION);
-
- kafkaStreams.start();
- kafkaStreams1.start();
Review comment:
The start is async, and we didn't wait for it.
----------------------------------------------------------------
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
[GitHub] [kafka] wcarlson5 commented on a change in pull request #9629: KAFKA-10754: fix flaky tests by waiting kafka streams be in running state before assert
Posted by GitBox <gi...@apache.org>.
wcarlson5 commented on a change in pull request #9629:
URL: https://github.com/apache/kafka/pull/9629#discussion_r528992986
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/StreamsUncaughtExceptionHandlerIntegrationTest.java
##########
@@ -110,83 +110,45 @@ public void teardown() throws IOException {
}
@Test
- public void shouldShutdownThreadUsingOldHandler() throws Exception {
+ public void shouldShutdownThreadUsingOldHandler() throws InterruptedException {
try (final KafkaStreams kafkaStreams = new KafkaStreams(builder.build(), properties)) {
- final CountDownLatch latch = new CountDownLatch(1);
final AtomicBoolean flag = new AtomicBoolean(false);
kafkaStreams.setUncaughtExceptionHandler((t, e) -> flag.set(true));
StreamsTestUtils.startKafkaStreamsAndWaitForRunningState(kafkaStreams);
-
produceMessages(0L, inputTopic, "A");
- waitForApplicationState(Collections.singletonList(kafkaStreams), KafkaStreams.State.ERROR, Duration.ofSeconds(15));
TestUtils.waitForCondition(flag::get, "Handler was called");
- assertThat(processorValueCollector.size(), equalTo(2));
Review comment:
as above
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/StreamsUncaughtExceptionHandlerIntegrationTest.java
##########
@@ -97,7 +97,7 @@ public void setup() {
mkEntry(StreamsConfig.BOOTSTRAP_SERVERS_CONFIG, CLUSTER.bootstrapServers()),
mkEntry(StreamsConfig.APPLICATION_ID_CONFIG, appId),
mkEntry(StreamsConfig.STATE_DIR_CONFIG, TestUtils.tempDirectory().getPath()),
- mkEntry(StreamsConfig.NUM_STREAM_THREADS_CONFIG, 2),
+ mkEntry(StreamsConfig.NUM_STREAM_THREADS_CONFIG, 1),
Review comment:
Yes, Both the old handler test and the close client should have 2 threads. We need to ensure that after a rebalance the old handler has attempted the process the record twice and the client shutdown only once. We can not be sure of that with only one thread.
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/StreamsUncaughtExceptionHandlerIntegrationTest.java
##########
@@ -110,83 +110,45 @@ public void teardown() throws IOException {
}
@Test
- public void shouldShutdownThreadUsingOldHandler() throws Exception {
+ public void shouldShutdownThreadUsingOldHandler() throws InterruptedException {
try (final KafkaStreams kafkaStreams = new KafkaStreams(builder.build(), properties)) {
- final CountDownLatch latch = new CountDownLatch(1);
final AtomicBoolean flag = new AtomicBoolean(false);
kafkaStreams.setUncaughtExceptionHandler((t, e) -> flag.set(true));
StreamsTestUtils.startKafkaStreamsAndWaitForRunningState(kafkaStreams);
-
produceMessages(0L, inputTopic, "A");
- waitForApplicationState(Collections.singletonList(kafkaStreams), KafkaStreams.State.ERROR, Duration.ofSeconds(15));
TestUtils.waitForCondition(flag::get, "Handler was called");
+ waitForApplicationState(Collections.singletonList(kafkaStreams), KafkaStreams.State.ERROR, DEFAULT_DURATION);
Review comment:
The order is not really that important here, either way works
----------------------------------------------------------------
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
[GitHub] [kafka] ableegoldman commented on a change in pull request #9629: KAFKA-10754: fix flaky tests by waiting kafka streams be in running state before assert
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #9629:
URL: https://github.com/apache/kafka/pull/9629#discussion_r528943389
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/StreamsUncaughtExceptionHandlerIntegrationTest.java
##########
@@ -110,83 +110,45 @@ public void teardown() throws IOException {
}
@Test
- public void shouldShutdownThreadUsingOldHandler() throws Exception {
+ public void shouldShutdownThreadUsingOldHandler() throws InterruptedException {
try (final KafkaStreams kafkaStreams = new KafkaStreams(builder.build(), properties)) {
- final CountDownLatch latch = new CountDownLatch(1);
final AtomicBoolean flag = new AtomicBoolean(false);
kafkaStreams.setUncaughtExceptionHandler((t, e) -> flag.set(true));
StreamsTestUtils.startKafkaStreamsAndWaitForRunningState(kafkaStreams);
-
produceMessages(0L, inputTopic, "A");
- waitForApplicationState(Collections.singletonList(kafkaStreams), KafkaStreams.State.ERROR, Duration.ofSeconds(15));
TestUtils.waitForCondition(flag::get, "Handler was called");
- assertThat(processorValueCollector.size(), equalTo(2));
Review comment:
@wcarlson5 for example, this test probably should have multiple threads, right?
##########
File path: streams/src/main/java/org/apache/kafka/streams/KafkaStreams.java
##########
@@ -379,13 +379,12 @@ public void setUncaughtExceptionHandler(final Thread.UncaughtExceptionHandler un
}
/**
- * Set the handler invoked when an {@link StreamsConfig#NUM_STREAM_THREADS_CONFIG internal thread}
+ * Set the handler invoked when an {@link StreamsConfig#NUM_STREAM_THREADS_CONFIG} internal thread
Review comment:
I think this was actually correct as it was (and ditto for the above). One alternative suggestion:
```suggestion
* Set the handler invoked when an internal {@link StreamsConfig#NUM_STREAM_THREADS_CONFIG stream thread}
```
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/StreamsUncaughtExceptionHandlerIntegrationTest.java
##########
@@ -97,7 +97,7 @@ public void setup() {
mkEntry(StreamsConfig.BOOTSTRAP_SERVERS_CONFIG, CLUSTER.bootstrapServers()),
mkEntry(StreamsConfig.APPLICATION_ID_CONFIG, appId),
mkEntry(StreamsConfig.STATE_DIR_CONFIG, TestUtils.tempDirectory().getPath()),
- mkEntry(StreamsConfig.NUM_STREAM_THREADS_CONFIG, 2),
+ mkEntry(StreamsConfig.NUM_STREAM_THREADS_CONFIG, 1),
Review comment:
Hey @wcarlson5 , can you take a look at this? If we change the default number of threads to 1 will we be reducing test coverage or not testing the correct thing anymore?
FWIW I think for tests where the number of threads doesn't matter, we should default to 1. But I'm not sure which tests do/do not rely on using multiple stream threads
----------------------------------------------------------------
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
[GitHub] [kafka] ableegoldman commented on pull request #9629: KAFKA-10754: fix flaky tests by waiting kafka streams be in running state before assert
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on pull request #9629:
URL: https://github.com/apache/kafka/pull/9629#issuecomment-733257040
Merged to trunk
----------------------------------------------------------------
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
[GitHub] [kafka] showuon commented on pull request #9629: KAFKA-10754: fix flaky tests by waiting kafka streams be in running state before assert
Posted by GitBox <gi...@apache.org>.
showuon commented on pull request #9629:
URL: https://github.com/apache/kafka/pull/9629#issuecomment-732646473
@ableegoldman @wcarlson5 , thanks for the comments. Now I know why it sets the default treads to 2. So, to make the test more reliable, I'll do:
1. wait for the state become running (as before)
2. wait for the 1st time handler got called in current thread
3. wait for the 2nd time handler got called after rebalancing
4. wait for the stream state turned into ERROR state (as before)
This should make this test more reliable. What do you think?
----------------------------------------------------------------
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
[GitHub] [kafka] showuon commented on a change in pull request #9629: KAFKA-10754: fix flaky tests by waiting kafka streams be in running state before assert
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #9629:
URL: https://github.com/apache/kafka/pull/9629#discussion_r527719599
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/StreamsUncaughtExceptionHandlerIntegrationTest.java
##########
@@ -110,83 +110,45 @@ public void teardown() throws IOException {
}
@Test
- public void shouldShutdownThreadUsingOldHandler() throws Exception {
+ public void shouldShutdownThreadUsingOldHandler() throws InterruptedException {
try (final KafkaStreams kafkaStreams = new KafkaStreams(builder.build(), properties)) {
- final CountDownLatch latch = new CountDownLatch(1);
Review comment:
delete unused `latch`
----------------------------------------------------------------
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