-
Notifications
You must be signed in to change notification settings - Fork 15.4k
KAFKA-5565. Add a broker metric specifying the number of consumer gro… #3506
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,19 +82,57 @@ class GroupMetadataManager(brokerId: Int, | |
|
|
||
| this.logIdent = s"[GroupMetadataManager brokerId=$brokerId] " | ||
|
|
||
| newGauge("NumOffsets", | ||
| private def recreateGauge[T](name: String, gauge: Gauge[T]): Gauge[T] = { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure why we want to remove-then-recreate here? Isn't this a one-time call for the life time? |
||
| removeMetric(name) | ||
| newGauge(name, gauge) | ||
| } | ||
|
|
||
| recreateGauge("NumOffsets", | ||
| new Gauge[Int] { | ||
| def value = groupMetadataCache.values.map(group => { | ||
| group synchronized { group.numOffsets } | ||
| }).sum | ||
| } | ||
| ) | ||
| }) | ||
|
|
||
| newGauge("NumGroups", | ||
| val numGroupsGauge = recreateGauge("NumGroups", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, other tests usually just retrieve the data from the metrics registry as you said.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK... |
||
| new Gauge[Int] { | ||
| def value = groupMetadataCache.size | ||
| } | ||
| ) | ||
| }) | ||
|
|
||
| val numGroupsPreparingRebalanceGauge = recreateGauge("NumGroupsPreparingRebalance", | ||
| new Gauge[Int] { | ||
| def value(): Int = groupMetadataCache.values.count(group => { | ||
| group synchronized { group.currentState == PreparingRebalance } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can replace this with |
||
| }) | ||
| }) | ||
|
|
||
| val numGroupsCompletingRebalanceGauge = recreateGauge("NumGroupsCompletingRebalance", | ||
| new Gauge[Int] { | ||
| def value(): Int = groupMetadataCache.values.count(group => { | ||
| group synchronized { group.currentState == CompletingRebalance } | ||
| }) | ||
| }) | ||
|
|
||
| recreateGauge("NumGroupsStable", | ||
| new Gauge[Int] { | ||
| def value(): Int = groupMetadataCache.values.count(group => { | ||
| group synchronized { group.currentState == Stable } | ||
| }) | ||
| }) | ||
|
|
||
| recreateGauge("NumGroupsDead", | ||
| new Gauge[Int] { | ||
| def value(): Int = groupMetadataCache.values.count(group => { | ||
| group synchronized { group.currentState == Dead } | ||
| }) | ||
| }) | ||
|
|
||
| recreateGauge("NumGroupsEmpty", | ||
| new Gauge[Int] { | ||
| def value(): Int = groupMetadataCache.values.count(group => { | ||
| group synchronized { group.currentState == Empty } | ||
| }) | ||
| }) | ||
|
|
||
| def enableMetadataExpiration() { | ||
| scheduler.startup() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,7 @@ import kafka.common.OffsetAndMetadata | |
| import kafka.log.{Log, LogAppendInfo} | ||
| import kafka.server.{FetchDataInfo, KafkaConfig, LogOffsetMetadata, ReplicaManager} | ||
| import kafka.utils.TestUtils.fail | ||
| import kafka.utils.{KafkaScheduler, MockTime, TestUtils, ZkUtils} | ||
| import kafka.utils.{KafkaScheduler, Logging, MockTime, TestUtils, ZkUtils} | ||
| import org.apache.kafka.common.TopicPartition | ||
| import org.apache.kafka.common.protocol.Errors | ||
| import org.apache.kafka.common.record._ | ||
|
|
@@ -34,12 +34,14 @@ import org.junit.Assert.{assertEquals, assertFalse, assertTrue} | |
| import org.junit.{Before, Test} | ||
| import java.nio.ByteBuffer | ||
|
|
||
| import com.yammer.metrics.Metrics | ||
| import com.yammer.metrics.core.Gauge | ||
| import org.apache.kafka.common.internals.Topic | ||
|
|
||
| import scala.collection.JavaConverters._ | ||
| import scala.collection._ | ||
|
|
||
| class GroupMetadataManagerTest { | ||
| class GroupMetadataManagerTest extends Logging { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like this is not needed. |
||
|
|
||
| var time: MockTime = null | ||
| var replicaManager: ReplicaManager = null | ||
|
|
@@ -1394,4 +1396,25 @@ class GroupMetadataManagerTest { | |
| EasyMock.expect(replicaManager.nonOfflinePartition(groupTopicPartition)).andStubReturn(Some(partition)) | ||
| } | ||
|
|
||
| private def expectMetrics(manager: GroupMetadataManager, | ||
| expectedNumGroups: Int, | ||
| expectedNumGroupsPreparingRebalance: Int, | ||
| expectedNumGroupsCompletingRebalance: Int): Unit = { | ||
| assertEquals(expectedNumGroups, manager.numGroupsGauge.value) | ||
| assertEquals(expectedNumGroupsPreparingRebalance, manager.numGroupsPreparingRebalanceGauge.value) | ||
| assertEquals(expectedNumGroupsCompletingRebalance, manager.numGroupsCompletingRebalanceGauge.value) | ||
| } | ||
|
|
||
| @Test | ||
| def testMetrics() { | ||
| groupMetadataManager.cleanupGroupMetadata() | ||
| expectMetrics(groupMetadataManager, 0, 0, 0) | ||
| val group = new GroupMetadata("foo2", Stable) | ||
| groupMetadataManager.addGroup(group) | ||
| expectMetrics(groupMetadataManager, 1, 0, 0) | ||
| group.transitionTo(PreparingRebalance) | ||
| expectMetrics(groupMetadataManager, 1, 1, 0) | ||
| group.transitionTo(CompletingRebalance) | ||
| expectMetrics(groupMetadataManager, 1, 0, 1) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is basically:
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.