Skip to content

MINOR: Make ControllerIntegrationTest less flaky#5829

Merged
ijuma merged 1 commit intoapache:trunkfrom
stanislavkozlovski:minor-controller-integration-test-flakiness
Oct 24, 2018
Merged

MINOR: Make ControllerIntegrationTest less flaky#5829
ijuma merged 1 commit intoapache:trunkfrom
stanislavkozlovski:minor-controller-integration-test-flakiness

Conversation

@stanislavkozlovski
Copy link
Copy Markdown
Contributor

@stanislavkozlovski stanislavkozlovski commented Oct 23, 2018

ControllerIntegrationTest#waitUntilControllerEpoch sometimes fails with the following error:

java.util.NoSuchElementException: None.get
	at scala.None$.get(Option.scala:347)
	at scala.None$.get(Option.scala:345)
	at kafka.controller.ControllerIntegrationTest$$anonfun$waitUntilControllerEpoch$1.apply$mcZ$sp(ControllerIntegrationTest.scala:312)
	at kafka.utils.TestUtils$.waitUntilTrue(TestUtils.scala:779)
	at kafka.controller.ControllerIntegrationTest.waitUntilControllerEpoch(ControllerIntegrationTest.scala:312)
	at kafka.controller.ControllerIntegrationTest.testEmptyCluster(ControllerIntegrationTest.scala:51)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	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.runLeaf(ParentRunner.java:325)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	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.run(ParentRunner.java:363)
	at

I think that ensuring the controllerEpoch is defined (by implicitly retrying) is a way to remove this error and fix cases where something transient has caused the method to return None.

rajinisivaram
rajinisivaram reviewed Oct 23, 2018
View reviewed changes
Comment thread core/src/test/scala/unit/kafka/controller/ControllerIntegrationTest.scala Outdated
Copy link
Copy Markdown
Contributor

@rajinisivaram rajinisivaram Oct 23, 2018

Choose a reason for hiding this comment

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

Perhaps we should do this with a single waitUntilTrue, so we only do a get when isDefined?

Copy link
Copy Markdown
Member

@ijuma ijuma Oct 23, 2018

Choose a reason for hiding this comment

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

Seems like we don't want to do a .get at all. We can compare with Some(...) instead.

Copy link
Copy Markdown
Contributor Author

@stanislavkozlovski stanislavkozlovski Oct 23, 2018

Choose a reason for hiding this comment

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

Updated

ijuma
ijuma reviewed Oct 23, 2018
View reviewed changes
Comment thread core/src/test/scala/unit/kafka/controller/ControllerIntegrationTest.scala Outdated
Copy link
Copy Markdown
Member

@ijuma ijuma Oct 23, 2018

Choose a reason for hiding this comment

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

Why is AnyRef being passed as a parameter?

Copy link
Copy Markdown
Contributor Author

@stanislavkozlovski stanislavkozlovski Oct 24, 2018

Choose a reason for hiding this comment

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

getControllerEpoch returns a tuple of Option[(Int, Stat)]. Can we compare them in another way?

Copy link
Copy Markdown
Member

@ijuma ijuma Oct 24, 2018

Choose a reason for hiding this comment

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

This will never be true. What you want is zkClient.getControllerEpoch.map(_._1) == Some(epoch)

Copy link
Copy Markdown
Contributor Author

@stanislavkozlovski stanislavkozlovski Oct 24, 2018

Choose a reason for hiding this comment

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

That's true. I apparently haven't run the tests after that change ... embarrassing

I updated the PR now

ijuma
ijuma approved these changes Oct 24, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@ijuma ijuma left a comment

Choose a reason for hiding this comment

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

LGTM

@ijuma ijuma merged commit d2c870b into apache:trunk Oct 24, 2018
ijuma pushed a commit that referenced this pull request Oct 24, 2018
@stanislavkozlovski @ijuma
MINOR: Fix flaky assertion in ControllerIntegrationTest (#5829)
e6b2027 
`ControllerIntegrationTest#waitUntilControllerEpoch` sometimes fails with the following error:

```
java.util.NoSuchElementException: None.get
	at scala.None$.get(Option.scala:347)
	at scala.None$.get(Option.scala:345)
	at kafka.controller.ControllerIntegrationTest$$anonfun$waitUntilControllerEpoch$1.apply$mcZ$sp(ControllerIntegrationTest.scala:312)
	at kafka.utils.TestUtils$.waitUntilTrue(TestUtils.scala:779)
	at kafka.controller.ControllerIntegrationTest.waitUntilControllerEpoch(ControllerIntegrationTest.scala:312)
	at kafka.controller.ControllerIntegrationTest.testEmptyCluster(ControllerIntegrationTest.scala:51)
```

We should retry until the value is defined or it times out.

Reviewers: Ismael Juma <ismael@juma.me.uk>
ijuma pushed a commit that referenced this pull request Oct 24, 2018
@stanislavkozlovski @ijuma
MINOR: Fix flaky assertion in ControllerIntegrationTest (#5829)
03a2b26 
`ControllerIntegrationTest#waitUntilControllerEpoch` sometimes fails with the following error:

```
java.util.NoSuchElementException: None.get
	at scala.None$.get(Option.scala:347)
	at scala.None$.get(Option.scala:345)
	at kafka.controller.ControllerIntegrationTest$$anonfun$waitUntilControllerEpoch$1.apply$mcZ$sp(ControllerIntegrationTest.scala:312)
	at kafka.utils.TestUtils$.waitUntilTrue(TestUtils.scala:779)
	at kafka.controller.ControllerIntegrationTest.waitUntilControllerEpoch(ControllerIntegrationTest.scala:312)
	at kafka.controller.ControllerIntegrationTest.testEmptyCluster(ControllerIntegrationTest.scala:51)
```

We should retry until the value is defined or it times out.

Reviewers: Ismael Juma <ismael@juma.me.uk>
@ijuma
Copy link
Copy Markdown
Member

ijuma commented Oct 24, 2018

Merged to trunk, 2.1 and 2.0 branches.

pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
@stanislavkozlovski @pengxiaolong
MINOR: Fix flaky assertion in ControllerIntegrationTest (apache#5829)
506ae1d 
`ControllerIntegrationTest#waitUntilControllerEpoch` sometimes fails with the following error:

```
java.util.NoSuchElementException: None.get
	at scala.None$.get(Option.scala:347)
	at scala.None$.get(Option.scala:345)
	at kafka.controller.ControllerIntegrationTest$$anonfun$waitUntilControllerEpoch$1.apply$mcZ$sp(ControllerIntegrationTest.scala:312)
	at kafka.utils.TestUtils$.waitUntilTrue(TestUtils.scala:779)
	at kafka.controller.ControllerIntegrationTest.waitUntilControllerEpoch(ControllerIntegrationTest.scala:312)
	at kafka.controller.ControllerIntegrationTest.testEmptyCluster(ControllerIntegrationTest.scala:51)
```

We should retry until the value is defined or it times out.

Reviewers: Ismael Juma <ismael@juma.me.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rajinisivaram rajinisivaram rajinisivaram left review comments

@ijuma ijuma ijuma approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@stanislavkozlovski @ijuma @rajinisivaram