KAFKA-5135: Controller Health Metrics (KIP-143) by ijuma · Pull Request #2983 · apache/kafka

ijuma · 2017-05-05T16:30:14Z

No description provided.

ijuma · 2017-05-05T16:31:05Z

This is an initial PR to get some feedback on whether I'm on the right track. Need to write tests and verify if any additional work needs to be done to clean-up metrics. cc @onurkaraman @junrao

onurkaraman · 2017-05-05T16:49:16Z

core/src/main/scala/kafka/controller/ControllerChannelManager.scala

This is a concurrency bug. You should be synchronizing access to brokerStateInfo.

onurkaraman · 2017-05-05T16:52:32Z

core/src/main/scala/kafka/controller/KafkaController.scala

The ElectingLeader state seems somewhat out of place. Should this reflect that a broker change event is occurring?

The KIP says BrokerChange indeed. However, we have LeaderElectionRateAndTimeMs that seems to measure the same code path. So, I thought it would make sense to use a similar name.

onurkaraman · 2017-05-05T16:53:57Z

core/src/main/scala/kafka/server/KafkaRequestHandler.scala

The BrokerTopicStats refactoring causes the PR to be much larger than I think it needs to be. Can we move this change out into a separate cleanup PR?

The changes are mechanical, the diffstat is +483 −315 (which is manageable) and there's a separate commit for the refactoring. So, I think we don't need a separate PR. Let's see what @junrao says.

asfbot · 2017-05-05T19:32:05Z

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

asfbot · 2017-05-05T19:33:35Z

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

asfbot · 2017-05-05T19:33:44Z

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

asfbot · 2017-05-05T23:07:40Z

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

asfbot · 2017-05-05T23:07:43Z

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

asfbot · 2017-05-05T23:07:47Z

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

junrao · 2017-05-06T01:14:23Z

@ijuma : Thanks for the patch. The general approach looks good.

asfbot · 2017-05-06T16:23:25Z

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

asfbot · 2017-05-06T17:09:47Z

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

asfbot · 2017-05-06T17:22:56Z

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

asfbot · 2017-05-06T18:52:14Z

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

asfbot · 2017-05-06T19:11:58Z

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

asfbot · 2017-05-06T19:30:06Z

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

asfbot · 2017-05-08T00:12:33Z

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

asfbot · 2017-05-08T00:13:49Z

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

asfbot · 2017-05-08T00:47:00Z

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

asfbot · 2017-05-15T00:51:28Z

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

asfbot · 2017-05-15T01:21:17Z

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

asfbot · 2017-05-15T19:10:13Z

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

asfbot · 2017-05-15T19:16:53Z

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

asfbot · 2017-05-15T22:35:00Z

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

asfbot · 2017-05-15T22:58:15Z

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

We don’t use any functionality from `AtomicInteger`, we simply set and read values.

…at need it Reducing exposure of fields from KafkaController reduces coupling.

asfbot · 2017-05-23T09:28:23Z

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

ijuma · 2017-05-23T09:30:31Z

Thanks for the review @junrao. I addressed your feedback. Also, while removing the controllerContext.stats.leaderElectionTimer.time block (which is now handled via the BrokerChange state metric), I noticed that some events had a redundant try/catch block (since ControllerEventManager.process also does the same). I've removed them as well.

asfbot · 2017-05-23T10:33:59Z

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

asfbot · 2017-05-23T10:40:29Z

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

junrao

@ijuma : Thanks for the updated patch. LGTM. Just one minor comment.

junrao · 2017-05-23T21:33:31Z

core/src/test/scala/unit/kafka/controller/ControllerEventManagerTest.scala

+    EasyMock.expect(eventMock.process()).andAnswer(new IAnswer[Unit]() {
+      def answer(): Unit = {
+        while (processing)
+          Thread.`yield`()


Since the behavior of Thread.yield() can be platform dependent, would it be better to change processing to sth like a CountDownLatch()?

That's a good suggestion. Both are correct (even if Thread.yield is a no-op, it won't matter here), but CountDownLatch is more concise and shows intent better. Updated the PR.

asfbot · 2017-05-23T22:47:13Z

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

junrao · 2017-05-23T23:12:49Z

@ijuma : Thanks for the patch. LGTM. I will let you merge this when the tests complete.

asfbot · 2017-05-23T23:28:14Z

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

Author: Ismael Juma <ismael@juma.me.uk> Reviewers: Jun Rao <junrao@gmail.com>, Onur Karaman <okaraman@linkedin.com> Closes #2983 from ijuma/kafka-5135-controller-health-metrics-kip-143 (cherry picked from commit 516d845) Signed-off-by: Ismael Juma <ismael@juma.me.uk>

onurkaraman reviewed May 5, 2017

View reviewed changes

ijuma force-pushed the kafka-5135-controller-health-metrics-kip-143 branch from bcc18df to 0217270 Compare May 6, 2017 15:43

ijuma force-pushed the kafka-5135-controller-health-metrics-kip-143 branch from 0217270 to 59267fb Compare May 6, 2017 17:36

ijuma force-pushed the kafka-5135-controller-health-metrics-kip-143 branch from 59267fb to c8cfd2f Compare May 7, 2017 23:04

ijuma force-pushed the kafka-5135-controller-health-metrics-kip-143 branch from c8cfd2f to d467ca4 Compare May 15, 2017 00:01

ijuma force-pushed the kafka-5135-controller-health-metrics-kip-143 branch from d467ca4 to 6227f86 Compare May 15, 2017 18:23

ijuma force-pushed the kafka-5135-controller-health-metrics-kip-143 branch from 6227f86 to 35bc136 Compare May 15, 2017 21:37

ijuma force-pushed the kafka-5135-controller-health-metrics-kip-143 branch from 7dc99cd to 0e2df6a Compare May 23, 2017 09:28

ijuma added 16 commits May 23, 2017 10:28

Add timer metrics

9a5fa0f

Code clean-ups

9e4d45a

Add controller state metric

40fd98a

Use volatile variables instead of AtomicInteger and other clean-ups

07b9577

We don’t use any functionality from `AtomicInteger`, we simply set and read values.

Add ControllerChannelManager queue size metrics

35b0573

Add FailedIsrUpdatesPerSec metric

6163bf3

Hold brokerLock while accessing brokerStateInfo

7225aa5

Include KIP updates and use a hopefully better approach.

aeddc8e

Extract ControllerEventManager for easier testing

7459cbd

Make KafkaController.eventManager private and pass it to classes th…

7f697a0

…at need it Reducing exposure of fields from KafkaController reduces coupling.

Add unit test and modify an existing integration test

eed9e47

Rename some controller states so that they are all nouns

c24abf8

Don’t duplicate existing LeaderElectionRateAndTimeMs metric

f996dd2

Remove comment that was added by mistake

13d120f

Catch and log if ControllerEventManager listener throws exception

2181e0d

Remove redundant try/catch from some ControllerEvent implementations

0e2df6a

junrao reviewed May 23, 2017

View reviewed changes

Use CountDownLatch to simplify the code

99fb9a1

ijuma force-pushed the kafka-5135-controller-health-metrics-kip-143 branch from e2a1dc7 to 99fb9a1 Compare May 23, 2017 21:54

asfgit closed this in 516d845 May 23, 2017

ijuma deleted the kafka-5135-controller-health-metrics-kip-143 branch September 5, 2017 08:42

Comments

Conversation

ijuma commented May 5, 2017

Uh oh!

ijuma commented May 5, 2017

Uh oh!

onurkaraman May 5, 2017

Choose a reason for hiding this comment

Uh oh!

onurkaraman May 5, 2017

Choose a reason for hiding this comment

Uh oh!

ijuma May 5, 2017

Choose a reason for hiding this comment

Uh oh!

onurkaraman May 5, 2017

Choose a reason for hiding this comment

Uh oh!

ijuma May 5, 2017

Choose a reason for hiding this comment

Uh oh!

asfbot commented May 5, 2017

Uh oh!

asfbot commented May 5, 2017

Uh oh!

asfbot commented May 5, 2017

Uh oh!

asfbot commented May 5, 2017

Uh oh!

asfbot commented May 5, 2017

Uh oh!

asfbot commented May 5, 2017

Uh oh!

junrao commented May 6, 2017

Uh oh!

asfbot commented May 6, 2017

Uh oh!

asfbot commented May 6, 2017

Uh oh!

asfbot commented May 6, 2017

Uh oh!

asfbot commented May 6, 2017

Uh oh!

asfbot commented May 6, 2017

Uh oh!

asfbot commented May 6, 2017

Uh oh!

asfbot commented May 8, 2017

Uh oh!

asfbot commented May 8, 2017

Uh oh!

asfbot commented May 8, 2017

Uh oh!

asfbot commented May 15, 2017

Uh oh!

asfbot commented May 15, 2017

Uh oh!

asfbot commented May 15, 2017

Uh oh!

asfbot commented May 15, 2017

Uh oh!

asfbot commented May 15, 2017

Uh oh!

asfbot commented May 15, 2017

Uh oh!

asfbot commented May 23, 2017

Uh oh!

ijuma commented May 23, 2017

Uh oh!

asfbot commented May 23, 2017

Uh oh!

asfbot commented May 23, 2017

Uh oh!

junrao left a comment

Choose a reason for hiding this comment

Uh oh!

junrao May 23, 2017

Choose a reason for hiding this comment

Uh oh!

ijuma May 23, 2017