KAFKA-5565. Add a broker metric specifying the number of consumer gro… by cmccabe · Pull Request #3506 · apache/kafka

cmccabe · 2017-07-07T22:13:35Z

…up rebalances in progress

asfgit · 2017-07-07T22:27:03Z

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

asfgit · 2017-07-07T22:29:42Z

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

asfgit · 2017-07-08T00:02:19Z

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

asfgit · 2017-07-08T00:30:22Z

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

ijuma

Thanks for the PR. We need a KIP because metrics are a public API. Left a couple of minor comments as well.

ijuma · 2017-07-12T02:16:07Z

core/src/main/scala/kafka/coordinator/group/GroupMetadataManager.scala

How about:

groupMetadataCache.values.count { group => group synchronized { group.currentState == AwaitingSync } }

ijuma · 2017-07-12T02:17:34Z

core/src/main/scala/kafka/coordinator/group/GroupMetadataManager.scala

Do we really need this? Can we not do the clean up in the test? If we think this is important, we should probably do it for all the gauges in KafkaMetricsGroup, but I am not sure if we need it.

The issue is basically:

GroupMetadataManager1 is created. It calls newGauge("NumOffsets"), which registers a metric with the global registry and returns it.

GroupMetadataManager2 is created. It calls newGauge("NumOffsets"). However, this function looks at the global registry and finds that there is already a metric there with that name. So it returns the gauge created in step Switch to using scala 2.9.2 #1.

The step Switch to using scala 2.9.2 #1 gauge is bound to the GroupMetadataManager object from step Switch to using scala 2.9.2 #1, not the one from step KAFKA-294 #2. So the tests fail because their modifications to GroupMetadataManager have no effect on the metrics.

The best way to solve this would probably be to explicitly pass the metric registry to the constructor of GroupMetadataManager, rather than relying on magical global variables. We could perhaps have it default to the global registry if no value was passed.

cmccabe · 2017-07-20T17:54:22Z

KIP:

https://cwiki.apache.org/confluence/display/KAFKA/KIP-180%3A+Add+a+broker+metric+specifying+the+number+of+consumer+group+rebalances+in+progress

asfgit · 2017-07-20T19:09:04Z

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

asfgit · 2017-07-20T19:37:30Z

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

ijuma · 2017-09-29T03:05:15Z

@hachikuji, maybe you can review this simple PR.

hachikuji · 2017-10-02T18:17:34Z

Discussed with @cmccabe offline. Seems like we may have lost some of the updates during rebase since this seems to reflect an old version of the KIP.

…up rebalances in progress

cmccabe · 2017-10-04T20:34:46Z

Sorry about the mixup. This should be the latest version, which reflects all KIP discussion.

hachikuji

Thanks for the patch. Just a couple comments.

hachikuji · 2017-10-05T20:44:24Z

core/src/main/scala/kafka/coordinator/group/GroupMetadataManager.scala

+  val numGroupsPreparingRebalanceGauge = recreateGauge("NumGroupsPreparingRebalance",
+    new Gauge[Int] {
+      def value(): Int = groupMetadataCache.values.count(group => {
+        group synchronized { group.currentState == PreparingRebalance }


You can replace this with group.is(PreparingRebalance). Similarly for the others.

hachikuji · 2017-10-05T20:51:18Z

core/src/main/scala/kafka/coordinator/group/GroupMetadataManager.scala

+    })

-  newGauge("NumGroups",
+  val numGroupsGauge = recreateGauge("NumGroups",


I'm not sure these need to be fields. It's a little more work, but we can pull the metric instances out of the metric registry in the test case.

Also, should we update the test case to cover all the metrics?

Yeah, other tests usually just retrieve the data from the metrics registry as you said.

ijuma · 2017-10-06T09:49:43Z

core/src/test/scala/unit/kafka/coordinator/group/GroupMetadataManagerTest.scala

 import scala.collection._

-class GroupMetadataManagerTest {
+class GroupMetadataManagerTest extends Logging {


Seems like this is not needed.

cmccabe · 2017-10-06T19:13:25Z

retest this please

cmccabe · 2017-10-06T19:15:57Z

Looks like Jenkins is having some issues

Caused: java.io.IOException: remote file operation failed: /home/jenkins/jenkins-slave/workspace/kafka-pr-jdk7-scala2.11 at hudson.remoting.Channel@1117e8b9:ubuntu-1
   at hudson.FilePath.act(FilePath.java:994)
   at hudson.FilePath.act(FilePath.java:976)
   at hudson.tasks.junit.JUnitParser.parseResult(JUnitParser.java:103)
   at hudson.tasks.junit.JUnitResultArchiver.parse(JUnitResultArchiver.java:128)
   at hudson.tasks.junit.JUnitResultArchiver.perform(JUnitResultArchiver.java:149)
   at hudson.tasks.BuildStepCompatibilityLayer.perform(BuildStepCompatibilityLayer.java:81)
   at hudson.tasks.BuildStepMonitor$1.perform(BuildStepMonitor.java:20)
   at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:736)
   at hudson.model.AbstractBuild$AbstractBuildExecution.performAllBuildSteps(AbstractBuild.java:682)
   at hudson.model.Build$BuildExecution.post2(Build.java:186)
   at hudson.model.AbstractBuild$AbstractBuildExecution.post(AbstractBuild.java:627)
   at hudson.model.Run.execute(Run.java:1762)
   at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:43)
   at hudson.model.ResourceController.execute(ResourceController.java:97)
   at hudson.model.Executor.run(Executor.java:419)
Adding one-line test results to commit status...
Setting status of 882c039b4ce2e5ff4c57ad96f3d136b9fa716eeb to FAILURE with url https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/8731/ and message: 'FAILURE

cmccabe · 2017-10-06T22:16:10Z

Retest this please

hachikuji · 2017-10-07T00:10:57Z

@cmccabe The one test failure seems legitimate.

guozhangwang · 2017-10-07T01:46:31Z

retest this please

guozhangwang · 2017-10-07T01:47:31Z

core/src/main/scala/kafka/coordinator/group/GroupMetadataManager.scala

  this.logIdent = s"[GroupMetadataManager brokerId=$brokerId] "

-  newGauge("NumOffsets",
+  private def recreateGauge[T](name: String, gauge: Gauge[T]): Gauge[T] = {


Not sure why we want to remove-then-recreate here? Isn't this a one-time call for the life time?

guozhangwang · 2017-10-09T16:29:04Z

LGTM. Merged to trunk.

…up rebalances in progress …up rebalances in progress Author: Colin P. Mccabe <cmccabe@confluent.io> Reviewers: Ismael Juma <ismael@juma.me.uk>, Jason Gustafson <jason@confluent.io>, Guozhang Wang <wangguoz@gmail.com> Closes apache#3506 from cmccabe/KAFKA-5565

cmccabe force-pushed the KAFKA-5565 branch from b243cc5 to 707c7d7 Compare July 7, 2017 23:03

ijuma reviewed Jul 12, 2017

View reviewed changes

cmccabe force-pushed the KAFKA-5565 branch from 7d763c5 to 26cf989 Compare September 26, 2017 16:34

KAFKA-5565. Add a broker metric specifying the number of consumer gro…

af96b2d

…up rebalances in progress

cmccabe force-pushed the KAFKA-5565 branch from 26cf989 to af96b2d Compare October 4, 2017 20:34

hachikuji reviewed Oct 5, 2017

View reviewed changes

ijuma reviewed Oct 6, 2017

View reviewed changes

Fixes

882c039

guozhangwang reviewed Oct 7, 2017

View reviewed changes

Fix test

bda46a6

asfgit closed this in 6d6080f Oct 9, 2017

cmccabe deleted the KAFKA-5565 branch May 20, 2019 18:45

hudeqi mentioned this pull request Jun 28, 2023

KAFKA-15129;[2/N] Remove metrics in GroupMetadataManager when shutdown #13926

Closed

Comments

Conversation

cmccabe commented Jul 7, 2017

Uh oh!

asfgit commented Jul 7, 2017

Uh oh!

asfgit commented Jul 7, 2017

Uh oh!

asfgit commented Jul 8, 2017

Uh oh!

asfgit commented Jul 8, 2017

Uh oh!

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

cmccabe commented Jul 20, 2017

Uh oh!

asfgit commented Jul 20, 2017

Uh oh!

asfgit commented Jul 20, 2017

Uh oh!

ijuma commented Sep 29, 2017

Uh oh!

hachikuji commented Oct 2, 2017

Uh oh!

cmccabe commented Oct 4, 2017

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmccabe commented Oct 6, 2017

Uh oh!

cmccabe commented Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmccabe commented Oct 6, 2017

Uh oh!

hachikuji commented Oct 7, 2017

Uh oh!

guozhangwang commented Oct 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guozhangwang commented Oct 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cmccabe commented Oct 6, 2017 •

edited

Loading