MINOR: onControllerResignation should be invoked if triggerControllerMove is called by ijuma · Pull Request #2935 · apache/kafka

ijuma · 2017-04-28T12:39:14Z

Also update the test to be simpler since we can use a mock event to simulate the issue
more easily (thanks Jun for the suggestion). This should fix two issues:

A transient test failure due to a NPE in ControllerFailoverTest.testMetadataUpdate:

Caused by: java.lang.NullPointerException
	at kafka.controller.ControllerBrokerRequestBatch.addUpdateMetadataRequestForBrokers(ControllerChannelManager.scala:338)
	at kafka.controller.KafkaController.sendUpdateMetadataRequest(KafkaController.scala:975)
	at kafka.controller.ControllerFailoverTest.testMetadataUpdate(ControllerFailoverTest.scala:141)

The test was creating an additional thread and it does not seem like it was doing the
appropriate synchronization (perhaps this became more of an issue after we changed
the Controller to be single-threaded and changed the locking)

Setting activeControllerId.set(-1) in triggerControllerMove causes Reelect not to invoke onControllerResignation. Among other things, this causes an IllegalStateException to be thrown when KafkaScheduler.startup is invoked for the second time without the corresponding shutdown. We now simply call onControllerResignation as part of triggerControllerMove.

Finally, I included a few clean-ups:

No longer update the broker state in onControllerFailover. This is no longer needed
since we removed the RunningAsController state (KAFKA-3761).
Trivial clean-ups in KafkaController
Removed unused parameter in ZkUtils.getPartitionLeaderAndIsrForTopics

ijuma · 2017-04-28T12:39:33Z

Link to previously failing Jenkins build: https://builds.apache.org/job/kafka-trunk-jdk8/1461/testReport/kafka.controller/ControllerFailoverTest/testMetadataUpdate/

asfbot · 2017-04-28T13:49:39Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/3277/
Test FAILed (JDK 7 and Scala 2.10).

ijuma · 2017-04-28T14:00:28Z

Review by @onurkaraman and @junrao.

asfbot · 2017-04-28T14:11:49Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/3282/
Test FAILed (JDK 8 and Scala 2.11).

asfbot · 2017-04-28T14:12:21Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/3273/
Test PASSed (JDK 8 and Scala 2.12).

onurkaraman · 2017-04-28T14:57:30Z

I think it would be worth figuring out whether the transiently failing tests were actually coming from bb663d0 and exactly why (as in, what in that patch caused the change).

The triggerControllerMove method introduced in bb663d0 is literally the former ZookeeperLeaderElector.resign but with a different method name. Here's the method in 0.10.2: https://github.com/apache/kafka/blob/0.10.2/core/src/main/scala/kafka/server/ZookeeperLeaderElector.scala#L109 for comparison. I just replaced all 4 calls of ZookeeperLeaderElector.resign with triggerControllerMove.

So there's no change stemming from triggerControllerMove. Maybe, as you said, a change happened in Reelect or ControllerChange, but this would be worth figuring out.

ijuma · 2017-04-28T15:38:59Z

@onurkaraman, thanks, I was hoping you'd be able to track down if it was indeed bb663d0. It looks like the underlying issue was there too:

[2017-04-28 16:14:51,760] ERROR Error while electing or becoming leader on broker 0 (kafka.server.ZookeeperLeaderElector:105)
java.lang.IllegalStateException: This scheduler has already been started!
	at kafka.utils.KafkaScheduler.startup(KafkaScheduler.scala:78)
	at kafka.controller.KafkaController.onControllerFailover(KafkaController.scala:351)
	at kafka.controller.KafkaController.$anonfun$controllerElector$1(KafkaController.scala:160)
	at kafka.server.ZookeeperLeaderElector.elect(ZookeeperLeaderElector.scala:85)
	at kafka.server.ZookeeperLeaderElector$LeaderChangeListener.$anonfun$handleDataDeleted$3(ZookeeperLeaderElector.scala:154)

The difference is that the NPE didn't seem to happen. Since the NPE requires a particular event ordering, it seems plausible that making it single threaded made this more likely than before.

onurkaraman · 2017-04-28T16:57:20Z

Interesting. I ran the test 60 times and they all passed.

> for i in {1..60}; do ./gradlew cleanTest core:test -Dtest.single=ControllerFailoverTest; done

I ran it against my local trunk which just went up to commit bb663d0 (KAFKA-5028).

ijuma · 2017-05-01T07:15:01Z

retest this please

asfbot · 2017-05-01T08:05:15Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/3321/
Test FAILed (JDK 7 and Scala 2.10).

asfbot · 2017-05-01T08:09:26Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/3326/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-05-01T08:39:40Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/3317/
Test FAILed (JDK 8 and Scala 2.12).

onurkaraman · 2017-05-01T16:45:16Z

I'm not sure if removing this line is the right move, as it will cause events to be processed by the controller even after triggering the controller move.

triggerControllerMove now just deletes the /controller znode. The Reelect corresponding to the /controller znode deletion will come behind events already in the queue. Since the PR removes the activeControllerId.set(-1) line, the controller will still think it's active and will process the queued events in between the current event and the Reelect event.

When I first saw the PR title, I thought you'd simply add a line calling onControllerResignation within triggerControllerMove. I think this makes more sense.

I tried adding onControllerResignation in that method first, but that causes another exception to be thrown. It may be that this test needs be rethought.

Yes, I agree that perhaps we should improve the test in ControllerFailoverTest. My understanding is that the test tries to simulate a case that the controller gets into an illegal state and tries to see if the controller can recover from it. But the way that it simulates the illegal state is quite involved. Perhaps we can do the following. (1) Consolidate all try/catch illegalStateException to ControllerEventThread.doWork(). (2) Change the test by inserting a MockEvent type that throws illegalStateException.

onurkaraman · 2017-05-01T17:25:19Z

By the way, your latest jenkins run still shows an NPE in the exact same spot in ControllerFailoverTest.testMetadataUpdate:
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/3321/testReport/junit/kafka.controller/ControllerFailoverTest/testMetadataUpdate/

I took a peek at the test itself. It's quite complicated. Do we know what it's actually trying to test?

ijuma · 2017-05-02T01:12:46Z

@onurkaraman, yes, I noticed that the latest Jenkins run triggered that NPE again. I looked a bit more and it seems that the test is racy as it calls non thread safe from a different thread. That may be the source of the NPE. So, more than one issue in play.

asfbot · 2017-05-02T01:17:33Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/3337/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-05-02T01:18:48Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/3346/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-05-02T01:26:36Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/3341/
Test PASSed (JDK 7 and Scala 2.10).

ijuma · 2017-05-25T14:15:24Z

It seems like this should be in the try/catch since it can throw an IllegalStateException and it seems like updateLeaderEpoch doesn't do anything with this field. But please double-check.

ijuma · 2017-05-25T14:20:09Z

@junrao @onurkaraman, here's another attempt that uses a mock event as suggested by Jun. I didn't move the IllegalStateException handling into ControllerEventManager because it also clears some state that the event manager doesn't have access to. I did extract a method, which seems like a reasonable compromise. Let me know what you think.

asfbot · 2017-05-25T15:12:09Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/4391/
Test FAILed (JDK 7 and Scala 2.11).

ijuma · 2017-05-25T15:33:14Z

Both of these should be waitUntilTrue as they happen after onControllerResigned

asfbot · 2017-05-25T15:40:15Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/4377/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-05-25T18:46:13Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/4401/
Test FAILed (JDK 7 and Scala 2.11).

asfbot · 2017-05-25T19:17:43Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/4387/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-05-26T01:33:53Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/4413/
Test PASSed (JDK 7 and Scala 2.11).

asfbot · 2017-05-26T01:49:57Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/4399/
Test PASSed (JDK 8 and Scala 2.12).

ijuma · 2017-05-26T08:59:20Z

retest this please

Tests passed, but running them again, just in case.

asfbot · 2017-05-26T09:56:22Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/4418/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-05-26T09:56:41Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/4432/
Test PASSed (JDK 7 and Scala 2.11).

junrao

@ijuma : Thanks for the patch. LGTM. Just a few minor comments.

junrao · 2017-05-27T00:38:05Z

missing license header

junrao · 2017-05-27T00:52:45Z

This assertion probably doesn't need to be in waitUntilTrue()?

junrao · 2017-05-27T01:05:25Z

It seems that we don't really need epochMap. previousEpoch could be obtained from the previous controller. If a broker is not a controller, the controller epoch is not valid.

…Move is called

ijuma · 2017-05-27T11:27:24Z

@junrao, I addressed your feedback and fixed a trivial merge conflict.

asfbot · 2017-05-27T12:47:27Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/4487/
Test PASSed (JDK 7 and Scala 2.11).

asfbot · 2017-05-27T12:48:07Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/4473/
Test FAILed (JDK 8 and Scala 2.12).

junrao · 2017-05-30T23:58:54Z

@ijuma : Thanks for the latest patch. LGTM

…Move is called Also update the test to be simpler since we can use a mock event to simulate the issue more easily (thanks Jun for the suggestion). This should fix two issues: 1. A transient test failure due to a NPE in ControllerFailoverTest.testMetadataUpdate: ```text Caused by: java.lang.NullPointerException at kafka.controller.ControllerBrokerRequestBatch.addUpdateMetadataRequestForBrokers(ControllerChannelManager.scala:338) at kafka.controller.KafkaController.sendUpdateMetadataRequest(KafkaController.scala:975) at kafka.controller.ControllerFailoverTest.testMetadataUpdate(ControllerFailoverTest.scala:141) ``` The test was creating an additional thread and it does not seem like it was doing the appropriate synchronization (perhaps this became more of an issue after we changed the Controller to be single-threaded and changed the locking) 2. Setting `activeControllerId.set(-1)` in `triggerControllerMove` causes `Reelect` not to invoke `onControllerResignation`. Among other things, this causes an `IllegalStateException` to be thrown when `KafkaScheduler.startup` is invoked for the second time without the corresponding `shutdown`. We now simply call `onControllerResignation` as part of `triggerControllerMove`. Finally, I included a few clean-ups: 1. No longer update the broker state in `onControllerFailover`. This is no longer needed since we removed the `RunningAsController` state (KAFKA-3761). 2. Trivial clean-ups in KafkaController 3. Removed unused parameter in `ZkUtils.getPartitionLeaderAndIsrForTopics` Author: Ismael Juma <ismael@juma.me.uk> Reviewers: Jun Rao <junrao@gmail.com> Closes #2935 from ijuma/on-controller-resignation-if-trigger-controller-move (cherry picked from commit 6021618) Signed-off-by: Jun Rao <junrao@gmail.com>

onurkaraman reviewed May 1, 2017

View reviewed changes

ijuma force-pushed the on-controller-resignation-if-trigger-controller-move branch from 241b989 to 919901e Compare May 2, 2017 00:12

ijuma force-pushed the on-controller-resignation-if-trigger-controller-move branch from 919901e to c188e61 Compare May 25, 2017 14:12

ijuma commented May 25, 2017

View reviewed changes

ijuma force-pushed the on-controller-resignation-if-trigger-controller-move branch 2 times, most recently from 6e922ca to 8e5d282 Compare May 25, 2017 14:18

ijuma commented May 25, 2017

View reviewed changes

junrao reviewed May 27, 2017

View reviewed changes

ijuma added 6 commits May 27, 2017 12:26

MINOR: onControllerResignation should be invoked if triggerController…

036b7f8

…Move is called

A few trivial clean-ups in KafkaController

e95a83f

Remove unused parameter in getPartitionLeaderAndIsrForTopics

f9e8996

Fix assertion in ControllerFailoverTest

d3ce48b

Really fix test

b3e7b00

Address review feedback

e603ee9

ijuma force-pushed the on-controller-resignation-if-trigger-controller-move branch from fa7405f to e603ee9 Compare May 27, 2017 11:27

asfgit closed this in 6021618 May 31, 2017

ijuma deleted the on-controller-resignation-if-trigger-controller-move branch June 18, 2017 09:30

ijuma mentioned this pull request Nov 27, 2019

MINOR: Refactor controller partition reassignment logic into separate class #7339

Closed

Conversation

ijuma commented Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ijuma commented Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asfbot commented Apr 28, 2017

Uh oh!

ijuma commented Apr 28, 2017

Uh oh!

asfbot commented Apr 28, 2017

Uh oh!

asfbot commented Apr 28, 2017

Uh oh!

onurkaraman commented Apr 28, 2017

Uh oh!

ijuma commented Apr 28, 2017

Uh oh!

onurkaraman commented Apr 28, 2017

Uh oh!

ijuma commented May 1, 2017

Uh oh!

asfbot commented May 1, 2017

Uh oh!

asfbot commented May 1, 2017

Uh oh!

asfbot commented May 1, 2017

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!

onurkaraman commented May 1, 2017

Uh oh!

ijuma commented May 2, 2017

Uh oh!

asfbot commented May 2, 2017

Uh oh!

asfbot commented May 2, 2017

Uh oh!

asfbot commented May 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented May 25, 2017

Uh oh!

asfbot commented May 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asfbot commented May 25, 2017

Uh oh!

asfbot commented May 25, 2017

Uh oh!

asfbot commented May 25, 2017

Uh oh!

asfbot commented May 26, 2017

Uh oh!

asfbot commented May 26, 2017

Uh oh!

ijuma commented May 26, 2017

Uh oh!

asfbot commented May 26, 2017

Uh oh!

asfbot commented May 26, 2017

Uh oh!

junrao 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!

ijuma commented May 27, 2017

Uh oh!

ijuma commented Apr 28, 2017 •

edited

Loading

ijuma commented Apr 28, 2017 •

edited

Loading