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/06/10 02:51:08 UTC
[GitHub] [kafka] guozhangwang opened a new pull request #8843: KAFKA-9991: Fix flaky unit tests
guozhangwang opened a new pull request #8843:
URL: https://github.com/apache/kafka/pull/8843
The latest commit #8254 on this test deleted all topics after each test, but the topic was actually shared among tests before. And after that we are relying on the less-reliable auto-topic generation to get the topic which makes the test flaky.
I'm now using different topics for different tests, also setting the app.id for tests differently.
### 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 commented on a change in pull request #8843: KAFKA-9991: Fix flaky unit tests
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #8843:
URL: https://github.com/apache/kafka/pull/8843#discussion_r438272343
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/KTableSourceTopicRestartIntegrationTest.java
##########
@@ -52,30 +51,28 @@
import java.util.Map;
import java.util.Properties;
import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ExecutionException;
@Category({IntegrationTest.class})
public class KTableSourceTopicRestartIntegrationTest {
private static final int NUM_BROKERS = 3;
private static final String SOURCE_TOPIC = "source-topic";
+ private static final Properties PRODUCER_CONFIG = new Properties();
+ private static final Properties STREAMS_CONFIG = new Properties();
@ClassRule
public static final EmbeddedKafkaCluster CLUSTER = new EmbeddedKafkaCluster(NUM_BROKERS);
+
private final Time time = CLUSTER.time;
- private KafkaStreams streamsOne;
private final StreamsBuilder streamsBuilder = new StreamsBuilder();
private final Map<String, String> readKeyValues = new ConcurrentHashMap<>();
- private static final Properties PRODUCER_CONFIG = new Properties();
- private static final Properties STREAMS_CONFIG = new Properties();
+ private String sourceTopic;
+ private KafkaStreams streamsOne;
Review comment:
super nit: can we rename to `streams1` to be consistent with other tests?
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/KTableSourceTopicRestartIntegrationTest.java
##########
@@ -96,8 +93,13 @@ public static void setUpBeforeAllTests() throws Exception {
public TestName testName = new TestName();
@Before
- public void before() {
- final KTable<String, String> kTable = streamsBuilder.table(SOURCE_TOPIC, Materialized.as("store"));
+ public void before() throws Exception {
+ sourceTopic = SOURCE_TOPIC + "-" + testName.getMethodName();
+ CLUSTER.createTopic(sourceTopic);
+
+ STREAMS_CONFIG.put(StreamsConfig.APPLICATION_ID_CONFIG, "ktable-restore-from-source-" + testName.getMethodName());
Review comment:
nit: use `IntegrationTestUtils#safeUniqueTestName` for the name
----------------------------------------------------------------
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] guozhangwang commented on pull request #8843: KAFKA-9991: Fix flaky unit tests
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on pull request #8843:
URL: https://github.com/apache/kafka/pull/8843#issuecomment-641688008
@mjsax @ableegoldman
----------------------------------------------------------------
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] abbccdda commented on a change in pull request #8843: KAFKA-9991: Fix flaky unit tests
Posted by GitBox <gi...@apache.org>.
abbccdda commented on a change in pull request #8843:
URL: https://github.com/apache/kafka/pull/8843#discussion_r437858956
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/KTableSourceTopicRestartIntegrationTest.java
##########
@@ -107,7 +109,6 @@ public void before() {
@After
public void after() throws Exception {
IntegrationTestUtils.purgeLocalStreamsState(STREAMS_CONFIG);
- CLUSTER.deleteAllTopicsAndWait(60000L);
Review comment:
Does the deletion logic affect the success of each test? If not, I would prefer to keep it to simplify the broker side log for each subsequent tests, such that we don't have a lot of unused topics.
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/KTableSourceTopicRestartIntegrationTest.java
##########
@@ -236,11 +237,7 @@ public void onRestoreStart(final TopicPartition topicPartition,
final String storeName,
final long startingOffset,
final long endingOffset) {
- try {
Review comment:
The original logic seems unnecessary?
----------------------------------------------------------------
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] guozhangwang commented on pull request #8843: KAFKA-9991: Fix flaky unit tests
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on pull request #8843:
URL: https://github.com/apache/kafka/pull/8843#issuecomment-642269028
Cherry-picked to 2.6.
----------------------------------------------------------------
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] guozhangwang commented on a change in pull request #8843: KAFKA-9991: Fix flaky unit tests
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #8843:
URL: https://github.com/apache/kafka/pull/8843#discussion_r438269033
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/KTableSourceTopicRestartIntegrationTest.java
##########
@@ -236,11 +237,7 @@ public void onRestoreStart(final TopicPartition topicPartition,
final String storeName,
final long startingOffset,
final long endingOffset) {
- try {
Review comment:
Yes, the callee would not throw actually.
----------------------------------------------------------------
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 #8843: KAFKA-9991: Fix flaky unit tests
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #8843:
URL: https://github.com/apache/kafka/pull/8843#discussion_r438271927
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/KTableSourceTopicRestartIntegrationTest.java
##########
@@ -107,7 +109,6 @@ public void before() {
@After
public void after() throws Exception {
IntegrationTestUtils.purgeLocalStreamsState(STREAMS_CONFIG);
- CLUSTER.deleteAllTopicsAndWait(60000L);
Review comment:
Yeah I agree 100% with Guozhang here, I wasted a lot of time trying to debug failures in the RestoreIntegrationTest that turned out to be due to reusing a single shared topic. +1 for using different topics (especially when we actually write data to them!)
----------------------------------------------------------------
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] guozhangwang commented on a change in pull request #8843: KAFKA-9991: Fix flaky unit tests
Posted by GitBox <gi...@apache.org>.
guozhangwang commented on a change in pull request #8843:
URL: https://github.com/apache/kafka/pull/8843#discussion_r438270138
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/KTableSourceTopicRestartIntegrationTest.java
##########
@@ -107,7 +109,6 @@ public void before() {
@After
public void after() throws Exception {
IntegrationTestUtils.purgeLocalStreamsState(STREAMS_CONFIG);
- CLUSTER.deleteAllTopicsAndWait(60000L);
Review comment:
Actually I think having different topics for different test cases have a merit for debugging -- in the past I've seen we unintentionally share topics between tests which is hard to trouble-shoot from the logs. Now with different topic names we would very easily notice if something went wrong.
----------------------------------------------------------------
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] guozhangwang merged pull request #8843: KAFKA-9991: Fix flaky unit tests
Posted by GitBox <gi...@apache.org>.
guozhangwang merged pull request #8843:
URL: https://github.com/apache/kafka/pull/8843
----------------------------------------------------------------
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