Skip to content

MINOR: fix race condition in KafkaStreamsTest#6185

Merged
guozhangwang merged 2 commits intoapache:trunkfrom
mjsax:minor-fix-race-condition
Jan 24, 2019
Merged

MINOR: fix race condition in KafkaStreamsTest#6185
guozhangwang merged 2 commits intoapache:trunkfrom
mjsax:minor-fix-race-condition

Conversation

@mjsax
Copy link
Copy Markdown
Member

@mjsax mjsax commented Jan 22, 2019

Saw failing build: https://builds.apache.org/blue/organizations/jenkins/kafka-trunk-jdk11/detail/kafka-trunk-jdk11/235/tests/

java.lang.AssertionError: expected:<6> but was:<5>
at org.junit.Assert.fail(Assert.java:88)
at org.junit.Assert.failNotEquals(Assert.java:834)
at org.junit.Assert.assertEquals(Assert.java:645)
at org.junit.Assert.assertEquals(Assert.java:631)
at org.apache.kafka.streams.KafkaStreamsTest.testStateOneThreadDeadButRebalanceFinish(KafkaStreamsTest.java:212)

Side "cleanup" to revert #6178 (was a try to stabilize the build, but did not resolve the issue) \cc @ijuma

@mjsax mjsax added the streams label Jan 22, 2019
@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented Jan 22, 2019

Call for review @guozhangwang @bbejeck @vvcephei @ableegoldman

mjsax
mjsax commented Jan 22, 2019
View reviewed changes
Comment thread streams/src/test/java/org/apache/kafka/streams/KafkaStreamsTest.java
Assert.assertEquals(6, stateListener.numChanges);
TestUtils.waitForCondition(
() -> stateListener.numChanges == 6,
"Streams never closed.");
Copy link
Copy Markdown
Member Author

@mjsax mjsax Jan 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual fix: as there are two state transitions, we cannot simply check for six transitions, because if we check too early, and only five transitions finished (or maybe none), stateListener.numChanges might still be at 4 or 5.

Copy link
Copy Markdown
Contributor

@guozhangwang guozhangwang Jan 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the catch! It was my bad for not capturing it last time I modified this test case.

vvcephei
vvcephei approved these changes Jan 22, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@vvcephei vvcephei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for this, @mjsax

@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented Jan 23, 2019

Retest this please.

guozhangwang
guozhangwang approved these changes Jan 24, 2019
View reviewed changes
Comment thread streams/src/test/java/org/apache/kafka/streams/KafkaStreamsTest.java
Assert.assertEquals(6, stateListener.numChanges);
TestUtils.waitForCondition(
() -> stateListener.numChanges == 6,
"Streams never closed.");
Copy link
Copy Markdown
Contributor

@guozhangwang guozhangwang Jan 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the catch! It was my bad for not capturing it last time I modified this test case.

@guozhangwang guozhangwang merged commit 86995ad into apache:trunk Jan 24, 2019
@mjsax mjsax deleted the minor-fix-race-condition branch January 24, 2019 07:21
@mjsax mjsax mentioned this pull request Jan 25, 2019
Merged
jarekr pushed a commit to confluentinc/kafka that referenced this pull request Apr 18, 2019
@dhruvilshah3
Merge remote-tracking branch 'ak/trunk' into cos-trunk
3e080e6 
* ak/trunk:
  MINOR: fix race condition in KafkaStreamsTest (apache#6185)
  KAFKA-4850: Enable bloomfilters (apache#6012)
  MINOR: ducker-ak: add down -f, avoid using a terminal in ducker test
  KAFKA-5117: Stop resolving externalized configs in Connect REST API
  MINOR: Cleanup handling of mixed transactional/idempotent records (apache#6172)
  KAFKA-7844: Use regular subproject for generator to fix *All targets (apache#6182)
  Fix Documentation for cleanup.policy is out of date (apache#6181)
  MINOR: increase timeouts for KafkaStreamsTest (apache#6178)
  MINOR: Rejoin split ssl principal mapping rules (apache#6099)
  MINOR: Handle case where connector status endpoints returns 404 (apache#6176)
  MINOR: Remove unused imports, exceptions, and values (apache#6117)
  KAFKA-3522: Add internal RecordConverter interface (apache#6150)
  Fix Javadoc of KafkaConsumer (apache#6155)
  KAFKA-6455: Extend CacheFlushListener to forward timestamp (apache#6147)
  MINOR: Log partition info when creating new request batch in controller (apache#6145)
  KAFKA-7652: Part I; Fix SessionStore's findSession(single-key) (apache#6134)
  MINOR: Remove the InvalidTopicException handling in InternalTopicManager (apache#6167)
  [KAFKA-7024] Rocksdb state directory should be created before opening the DB (apache#6138)
  MINOR:: Fix typos (apache#6079)
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
@mjsax @pengxiaolong
MINOR: fix race condition in KafkaStreamsTest (apache#6185)
41019ab 
Reviewers: John Roesler <john@confluent.io>, Guozhang Wang <guozhang@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vvcephei vvcephei vvcephei approved these changes

@guozhangwang guozhangwang guozhangwang approved these changes

Assignees

No one assigned

Labels

streams

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mjsax @vvcephei @guozhangwang