Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
chia7712 merged PR #15863: URL: https://github.com/apache/kafka/pull/15863 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
chiacyu commented on code in PR #15863: URL: https://github.com/apache/kafka/pull/15863#discussion_r1623229305 ## core/src/main/scala/kafka/log/LogCleaner.scala: ## @@ -223,8 +230,8 @@ class LogCleaner(initialConfig: CleanerConfig, info(s"Updating logCleanerIoMaxBytesPerSecond: $maxIoBytesPerSecond") throttler.updateDesiredRatePerSec(maxIoBytesPerSecond) } - -shutdown() +//call shutdownCleaners() instead of shutdown to avoid unnecessary deletion of metrics Review Comment: Thanks for the comments, applied, please take a look. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
rishiraj88 commented on code in PR #15863: URL: https://github.com/apache/kafka/pull/15863#discussion_r1613799763 ## core/src/main/scala/kafka/log/LogCleaner.scala: ## @@ -223,8 +230,8 @@ class LogCleaner(initialConfig: CleanerConfig, info(s"Updating logCleanerIoMaxBytesPerSecond: $maxIoBytesPerSecond") throttler.updateDesiredRatePerSec(maxIoBytesPerSecond) } - -shutdown() +//call shutdownCleaners() instead of shutdown to avoid unnecessary deletion of metrics Review Comment: I agree for the indentation. The code will then look even more uniformed. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
gaurav-narula commented on code in PR #15863: URL: https://github.com/apache/kafka/pull/15863#discussion_r1613630123 ## core/src/main/scala/kafka/log/LogCleaner.scala: ## @@ -223,8 +230,8 @@ class LogCleaner(initialConfig: CleanerConfig, info(s"Updating logCleanerIoMaxBytesPerSecond: $maxIoBytesPerSecond") throttler.updateDesiredRatePerSec(maxIoBytesPerSecond) } - -shutdown() +//call shutdownCleaners() instead of shutdown to avoid unnecessary deletion of metrics Review Comment: Nit: perhaps indent the comment -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
chia7712 commented on PR #15863: URL: https://github.com/apache/kafka/pull/15863#issuecomment-2128658669 @gaurav-narula this PR adopt your solution (https://github.com/apache/kafka/pull/15863#discussion_r1590313031) now, so it would be great to have your reviews before merging. thanks -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
chia7712 commented on PR #15863: URL: https://github.com/apache/kafka/pull/15863#issuecomment-2128654632 @chiacyu Could you please rebase PR to run QA with newest code? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
chia7712 commented on code in PR #15863: URL: https://github.com/apache/kafka/pull/15863#discussion_r1610219554 ## core/src/main/scala/kafka/log/LogCleaner.scala: ## @@ -223,8 +230,8 @@ class LogCleaner(initialConfig: CleanerConfig, info(s"Updating logCleanerIoMaxBytesPerSecond: $maxIoBytesPerSecond") throttler.updateDesiredRatePerSec(maxIoBytesPerSecond) } - -shutdown() +/* call shutdownCleaners() instead of shutdown to avoid unnecessary deletion of metrics */ Review Comment: please use `//` instead of `/*` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
chia7712 commented on code in PR #15863:
URL: https://github.com/apache/kafka/pull/15863#discussion_r1610083521
##
core/src/main/scala/kafka/log/LogCleaner.scala:
##
@@ -224,7 +232,7 @@ class LogCleaner(initialConfig: CleanerConfig,
throttler.updateDesiredRatePerSec(maxIoBytesPerSecond)
}
-shutdown()
+shutdownCleaners()
Review Comment:
Please add comments to say why we call `shutdownCleaners` instead of
`shutdown`
##
core/src/main/scala/kafka/log/LogCleaner.scala:
##
@@ -164,13 +164,21 @@ class LogCleaner(initialConfig: CleanerConfig,
/**
* Stop the background cleaner threads
*/
- def shutdown(): Unit = {
+ private[this] def shutdownCleaners(): Unit = {
info("Shutting down the log cleaner.")
+cleaners.foreach(_.shutdown())
+cleaners.clear()
+ }
+
+ /**
+ * Stop the background cleaner threads
+ */
+ def shutdown(): Unit = {
try {
- cleaners.foreach(_.shutdown())
- cleaners.clear()
+ shutdownCleaners()
} finally {
removeMetrics()
+ info("Shutting down the log cleaner.")
Review Comment:
this is duplicate to line#168
##
core/src/test/scala/unit/kafka/log/LogCleanerTest.scala:
##
@@ -1942,6 +1963,7 @@ class LogCleanerTest extends Logging {
logs = new Pool[TopicPartition, UnifiedLog](),
logDirFailureChannel = new LogDirFailureChannel(1),
time = time)
+logCleaner.startup()
Review Comment:
This is unnecessary now, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
chiacyu commented on code in PR #15863:
URL: https://github.com/apache/kafka/pull/15863#discussion_r1606776333
##
core/src/main/scala/kafka/log/LogCleaner.scala:
##
@@ -126,29 +126,34 @@ class LogCleaner(initialConfig: CleanerConfig,
private def maxOverCleanerThreads(f: CleanerThread => Double): Int =
cleaners.foldLeft(0.0d)((max: Double, thread: CleanerThread) =>
math.max(max, f(thread))).toInt
- /* a metric to track the maximum utilization of any thread's buffer in the
last cleaning */
- metricsGroup.newGauge(MaxBufferUtilizationPercentMetricName,
-() => maxOverCleanerThreads(_.lastStats.bufferUtilization) * 100)
-
- /* a metric to track the recopy rate of each thread's last cleaning */
- metricsGroup.newGauge(CleanerRecopyPercentMetricName, () => {
-val stats = cleaners.map(_.lastStats)
-val recopyRate = stats.iterator.map(_.bytesWritten).sum.toDouble /
math.max(stats.iterator.map(_.bytesRead).sum, 1)
-(100 * recopyRate).toInt
- })
-
- /* a metric to track the maximum cleaning time for the last cleaning from
each thread */
- metricsGroup.newGauge(MaxCleanTimeMetricName, () =>
maxOverCleanerThreads(_.lastStats.elapsedSecs))
-
- // a metric to track delay between the time when a log is required to be
compacted
- // as determined by max compaction lag and the time of last cleaner run.
- metricsGroup.newGauge(MaxCompactionDelayMetricsName,
-() =>
maxOverCleanerThreads(_.lastPreCleanStats.maxCompactionDelayMs.toDouble) / 1000)
-
- metricsGroup.newGauge(DeadThreadCountMetricName, () => deadThreadCount)
-
private[log] def deadThreadCount: Int = cleaners.count(_.isThreadFailed)
+ /**
+ * Activate metrics
+ */
+ private def activateMetrics(): Unit = {
Review Comment:
Thanks for the reminder. Would remove the function and restore back to
constructor.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
chia7712 commented on code in PR #15863:
URL: https://github.com/apache/kafka/pull/15863#discussion_r1606049924
##
core/src/main/scala/kafka/log/LogCleaner.scala:
##
@@ -126,29 +126,34 @@ class LogCleaner(initialConfig: CleanerConfig,
private def maxOverCleanerThreads(f: CleanerThread => Double): Int =
cleaners.foldLeft(0.0d)((max: Double, thread: CleanerThread) =>
math.max(max, f(thread))).toInt
- /* a metric to track the maximum utilization of any thread's buffer in the
last cleaning */
- metricsGroup.newGauge(MaxBufferUtilizationPercentMetricName,
-() => maxOverCleanerThreads(_.lastStats.bufferUtilization) * 100)
-
- /* a metric to track the recopy rate of each thread's last cleaning */
- metricsGroup.newGauge(CleanerRecopyPercentMetricName, () => {
-val stats = cleaners.map(_.lastStats)
-val recopyRate = stats.iterator.map(_.bytesWritten).sum.toDouble /
math.max(stats.iterator.map(_.bytesRead).sum, 1)
-(100 * recopyRate).toInt
- })
-
- /* a metric to track the maximum cleaning time for the last cleaning from
each thread */
- metricsGroup.newGauge(MaxCleanTimeMetricName, () =>
maxOverCleanerThreads(_.lastStats.elapsedSecs))
-
- // a metric to track delay between the time when a log is required to be
compacted
- // as determined by max compaction lag and the time of last cleaner run.
- metricsGroup.newGauge(MaxCompactionDelayMetricsName,
-() =>
maxOverCleanerThreads(_.lastPreCleanStats.maxCompactionDelayMs.toDouble) / 1000)
-
- metricsGroup.newGauge(DeadThreadCountMetricName, () => deadThreadCount)
-
private[log] def deadThreadCount: Int = cleaners.count(_.isThreadFailed)
+ /**
+ * Activate metrics
+ */
+ private def activateMetrics(): Unit = {
Review Comment:
We don't need this function now, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
chiacyu commented on code in PR #15863: URL: https://github.com/apache/kafka/pull/15863#discussion_r1605700828 ## core/src/main/scala/kafka/log/LogCleaner.scala: ## @@ -182,6 +183,27 @@ class LogCleaner(initialConfig: CleanerConfig, cleanerManager.removeMetrics() Review Comment: Oh, I got your point now. That make sense to me, will apply the change, please take a look. Thanks. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
chia7712 commented on code in PR #15863:
URL: https://github.com/apache/kafka/pull/15863#discussion_r1605365153
##
core/src/main/scala/kafka/log/LogCleaner.scala:
##
@@ -182,6 +183,27 @@ class LogCleaner(initialConfig: CleanerConfig,
cleanerManager.removeMetrics()
Review Comment:
> We can removed the cleanerManager.removeMetrics() then we should also
removed
[this](https://github.com/apache/kafka/blob/fafa3c76dc93f3258b2cea49dfd1dc7a724a213c/core/src/test/scala/unit/kafka/log/LogCleanerTest.scala#L96)
in the testRemoveMetricsOnClose().
My point was - we don't need to re-create the metrics after reconfiguration
- i.e in reconfiguration we SHOULD NOT remove the metrics. For example,
```scala
private[this] def shutdownCleaners(): Unit = {
info("Shutting down the log cleaner.")
cleaners.foreach(_.shutdown())
cleaners.clear()
}
/**
* Stop the background cleaner threads
*/
def shutdown(): Unit = {
try shutdownCleaners()
finally removeMetrics()
info("Shutting down the log cleaner.")
}
```
With above changes, in `reconfigure` we call `shutdownCleaners` instead of
`shutdown`. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
chiacyu commented on code in PR #15863: URL: https://github.com/apache/kafka/pull/15863#discussion_r1604267579 ## core/src/main/scala/kafka/log/LogCleaner.scala: ## @@ -182,6 +183,27 @@ class LogCleaner(initialConfig: CleanerConfig, cleanerManager.removeMetrics() Review Comment: I thought `testMetricsActiveAfterReconfiguration()` already test that the metrics are not removed after reconfiguration. We can removed the `cleanerManager.removeMetrics()` then we should also removed [this](https://github.com/apache/kafka/blob/fafa3c76dc93f3258b2cea49dfd1dc7a724a213c/core/src/test/scala/unit/kafka/log/LogCleanerTest.scala#L96) in the `testRemoveMetricsOnClose()`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
chia7712 commented on code in PR #15863: URL: https://github.com/apache/kafka/pull/15863#discussion_r1599475760 ## core/src/main/scala/kafka/log/LogCleaner.scala: ## @@ -182,6 +183,27 @@ class LogCleaner(initialConfig: CleanerConfig, cleanerManager.removeMetrics() Review Comment: > I reckon the metrics in LogCleanerManager would remained removed? Perhaps they shouldn't be removed while reconfiguring? @gaurav-narula that is a good point. @chiacyu Could you write ut to verify that metrics of `LogCleanerManager` are not removed after reconfiguration. Also, @gaurav-narula's suggestion is a better way I feel. Could you please give a try? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
chia7712 commented on code in PR #15863:
URL: https://github.com/apache/kafka/pull/15863#discussion_r1598340663
##
core/src/main/scala/kafka/log/LogCleaner.scala:
##
@@ -126,29 +126,34 @@ class LogCleaner(initialConfig: CleanerConfig,
private def maxOverCleanerThreads(f: CleanerThread => Double): Int =
cleaners.foldLeft(0.0d)((max: Double, thread: CleanerThread) =>
math.max(max, f(thread))).toInt
- /* a metric to track the maximum utilization of any thread's buffer in the
last cleaning */
- metricsGroup.newGauge(MaxBufferUtilizationPercentMetricName,
-() => maxOverCleanerThreads(_.lastStats.bufferUtilization) * 100)
-
- /* a metric to track the recopy rate of each thread's last cleaning */
- metricsGroup.newGauge(CleanerRecopyPercentMetricName, () => {
-val stats = cleaners.map(_.lastStats)
-val recopyRate = stats.iterator.map(_.bytesWritten).sum.toDouble /
math.max(stats.iterator.map(_.bytesRead).sum, 1)
-(100 * recopyRate).toInt
- })
-
- /* a metric to track the maximum cleaning time for the last cleaning from
each thread */
- metricsGroup.newGauge(MaxCleanTimeMetricName, () =>
maxOverCleanerThreads(_.lastStats.elapsedSecs))
-
- // a metric to track delay between the time when a log is required to be
compacted
- // as determined by max compaction lag and the time of last cleaner run.
- metricsGroup.newGauge(MaxCompactionDelayMetricsName,
-() =>
maxOverCleanerThreads(_.lastPreCleanStats.maxCompactionDelayMs.toDouble) / 1000)
-
- metricsGroup.newGauge(DeadThreadCountMetricName, () => deadThreadCount)
-
private[log] def deadThreadCount: Int = cleaners.count(_.isThreadFailed)
+ /**
+ * Activate metrics
+ */
+ private def activateMetrics():Unit = {
Review Comment:
`activateMetrics(): Unit`
##
core/src/main/scala/kafka/log/LogCleaner.scala:
##
@@ -159,6 +164,7 @@ class LogCleaner(initialConfig: CleanerConfig,
cleaners += cleaner
cleaner.start()
}
+activateMetrics();
Review Comment:
please remove `;`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
chiacyu commented on code in PR #15863:
URL: https://github.com/apache/kafka/pull/15863#discussion_r1597619075
##
core/src/main/scala/kafka/log/LogCleaner.scala:
##
@@ -101,6 +101,7 @@ class LogCleaner(initialConfig: CleanerConfig,
time: Time = Time.SYSTEM) extends Logging with
BrokerReconfigurable {
// Visible for test.
private[log] val metricsGroup = new KafkaMetricsGroup(this.getClass)
+ activateMetrics()
Review Comment:
Thanks for the reminder. Added the `startup()` back to test functions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
chia7712 commented on code in PR #15863:
URL: https://github.com/apache/kafka/pull/15863#discussion_r1597469917
##
core/src/main/scala/kafka/log/LogCleaner.scala:
##
@@ -101,6 +101,7 @@ class LogCleaner(initialConfig: CleanerConfig,
time: Time = Time.SYSTEM) extends Logging with
BrokerReconfigurable {
// Visible for test.
private[log] val metricsGroup = new KafkaMetricsGroup(this.getClass)
+ activateMetrics()
Review Comment:
> some of the test cases would fail. The metric would not be initialized. Is
there a better way to refactor that? thanks!
Could you please share the test cases to me? Maybe they do not call
`startup`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
chiacyu commented on code in PR #15863:
URL: https://github.com/apache/kafka/pull/15863#discussion_r1597401032
##
core/src/main/scala/kafka/log/LogCleaner.scala:
##
@@ -101,6 +101,7 @@ class LogCleaner(initialConfig: CleanerConfig,
time: Time = Time.SYSTEM) extends Logging with
BrokerReconfigurable {
// Visible for test.
private[log] val metricsGroup = new KafkaMetricsGroup(this.getClass)
+ activateMetrics()
Review Comment:
If we remove the activate function from the construction, some of the test
cases would fail. The metric would not be initialized. Is there a better way to
refactor that? thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
chia7712 commented on code in PR #15863:
URL: https://github.com/apache/kafka/pull/15863#discussion_r1592508554
##
core/src/main/scala/kafka/log/LogCleaner.scala:
##
@@ -101,6 +101,7 @@ class LogCleaner(initialConfig: CleanerConfig,
time: Time = Time.SYSTEM) extends Logging with
BrokerReconfigurable {
// Visible for test.
private[log] val metricsGroup = new KafkaMetricsGroup(this.getClass)
+ activateMetrics()
Review Comment:
As it gets called in `startup`, do we need to call it in construction?
##
core/src/test/scala/unit/kafka/log/LogCleanerTest.scala:
##
@@ -118,6 +118,23 @@ class LogCleanerTest extends Logging {
}
}
+ @Test
+ def testMetricsActiveAfterReconfiguration(): Unit = {
+val logCleaner = new LogCleaner(new CleanerConfig(true),
+ logDirs = Array(TestUtils.tempDir()),
+ logs = new Pool[TopicPartition, UnifiedLog](),
+ logDirFailureChannel = new LogDirFailureChannel(1),
+ time = time)
+
+try {
+ logCleaner.reconfigure(new KafkaConfig(TestUtils.createBrokerConfig(1,
"localhost:2181")),
+new KafkaConfig(TestUtils.createBrokerConfig(1, "localhost:2181")))
+
+ LogCleaner.MetricNames.foreach(name =>
assertNotNull(KafkaYammerMetrics.defaultRegistry.allMetrics().get(logCleaner.metricsGroup
Review Comment:
We can use `MetricName#getName` to simplify the code. For example:
```scala
val nonexistent =
LogCleaner.MetricNames.diff(KafkaYammerMetrics.defaultRegistry.allMetrics().keySet().asScala.map(_.getName))
assertEquals(0, nonexistent.size, s"$nonexistent should be existent")
```
##
core/src/test/scala/unit/kafka/log/LogCleanerTest.scala:
##
@@ -118,6 +118,23 @@ class LogCleanerTest extends Logging {
}
}
+ @Test
+ def testMetricsActiveAfterReconfiguration(): Unit = {
+val logCleaner = new LogCleaner(new CleanerConfig(true),
+ logDirs = Array(TestUtils.tempDir()),
+ logs = new Pool[TopicPartition, UnifiedLog](),
+ logDirFailureChannel = new LogDirFailureChannel(1),
+ time = time)
+
Review Comment:
Could you please add `startup` and then check the metrics get created?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
chiacyu commented on code in PR #15863: URL: https://github.com/apache/kafka/pull/15863#discussion_r1592516715 ## core/src/main/scala/kafka/log/LogCleaner.scala: ## @@ -159,6 +159,7 @@ class LogCleaner(initialConfig: CleanerConfig, cleaners += cleaner cleaner.start() } +activateMetrics(); Review Comment: Already added the test on the JIRA page: [KAFKA-16574](https://issues.apache.org/jira/browse/KAFKA-16574). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
chiacyu commented on code in PR #15863: URL: https://github.com/apache/kafka/pull/15863#discussion_r1592509827 ## core/src/main/scala/kafka/log/LogCleaner.scala: ## @@ -182,6 +183,27 @@ class LogCleaner(initialConfig: CleanerConfig, cleanerManager.removeMetrics() Review Comment: Yes, the metrics remained removed after reconfiguring, would it be a good idea to remove `cleanerManager.removeMetrics()` this line? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
chia7712 commented on code in PR #15863:
URL: https://github.com/apache/kafka/pull/15863#discussion_r1590455114
##
core/src/main/scala/kafka/log/LogCleaner.scala:
##
@@ -182,6 +183,27 @@ class LogCleaner(initialConfig: CleanerConfig,
cleanerManager.removeMetrics()
}
+ /**
+ * Activate metrics
+ */
+ def activateMetrics():Unit = {
+metricsGroup.newGauge(MaxBufferUtilizationPercentMetricName,
Review Comment:
agree. @chiacyu Could you Please avoid producing duplicate code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
soarez commented on code in PR #15863:
URL: https://github.com/apache/kafka/pull/15863#discussion_r1590415617
##
core/src/main/scala/kafka/log/LogCleaner.scala:
##
@@ -159,6 +159,7 @@ class LogCleaner(initialConfig: CleanerConfig,
cleaners += cleaner
cleaner.start()
}
+activateMetrics();
Review Comment:
Since there is no change to the existing definition of metrics, it seems
this will cause the metrics to be initialized twice.
##
core/src/main/scala/kafka/log/LogCleaner.scala:
##
@@ -182,6 +183,27 @@ class LogCleaner(initialConfig: CleanerConfig,
cleanerManager.removeMetrics()
}
+ /**
+ * Activate metrics
+ */
+ def activateMetrics():Unit = {
+metricsGroup.newGauge(MaxBufferUtilizationPercentMetricName,
+ () => maxOverCleanerThreads(_.lastStats.bufferUtilization) * 100)
+
+metricsGroup.newGauge(CleanerRecopyPercentMetricName, () => {
+ val stats = cleaners.map(_.lastStats)
+ val recopyRate = stats.iterator.map(_.bytesWritten).sum.toDouble /
math.max(stats.iterator.map(_.bytesRead).sum, 1)
+ (100 * recopyRate).toInt
+})
+
+metricsGroup.newGauge(MaxCleanTimeMetricName, () =>
maxOverCleanerThreads(_.lastStats.elapsedSecs))
+
+metricsGroup.newGauge(MaxCompactionDelayMetricsName,
+ () =>
maxOverCleanerThreads(_.lastPreCleanStats.maxCompactionDelayMs.toDouble) / 1000)
+
+metricsGroup.newGauge(DeadThreadCountMetricName, () => deadThreadCount)
Review Comment:
If we're moving the metrics here, please keep the existing comments.
##
core/src/main/scala/kafka/log/LogCleaner.scala:
##
@@ -159,6 +159,7 @@ class LogCleaner(initialConfig: CleanerConfig,
cleaners += cleaner
cleaner.start()
}
+activateMetrics();
Review Comment:
+1 Please include a test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
gaurav-narula commented on code in PR #15863: URL: https://github.com/apache/kafka/pull/15863#discussion_r1590315633 ## core/src/main/scala/kafka/log/LogCleaner.scala: ## @@ -159,6 +159,7 @@ class LogCleaner(initialConfig: CleanerConfig, cleaners += cleaner cleaner.start() } +activateMetrics(); Review Comment: Would be useful to add the test in the JIRA for posterity. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-16574: The metrics of LogCleaner disappear after reconfiguration [kafka]
gaurav-narula commented on code in PR #15863:
URL: https://github.com/apache/kafka/pull/15863#discussion_r1590313031
##
core/src/main/scala/kafka/log/LogCleaner.scala:
##
@@ -182,6 +183,27 @@ class LogCleaner(initialConfig: CleanerConfig,
cleanerManager.removeMetrics()
Review Comment:
I reckon the metrics in `LogCleanerManager` would remained removed? Perhaps
they shouldn't be removed while reconfiguring?
##
core/src/main/scala/kafka/log/LogCleaner.scala:
##
@@ -159,6 +159,7 @@ class LogCleaner(initialConfig: CleanerConfig,
cleaners += cleaner
cleaner.start()
}
+activateMetrics();
Review Comment:
Would be useful to add the test in the original JIRA for posterity.
##
core/src/main/scala/kafka/log/LogCleaner.scala:
##
@@ -182,6 +183,27 @@ class LogCleaner(initialConfig: CleanerConfig,
cleanerManager.removeMetrics()
}
+ /**
+ * Activate metrics
+ */
+ def activateMetrics():Unit = {
+metricsGroup.newGauge(MaxBufferUtilizationPercentMetricName,
Review Comment:
Perhaps we can remove the declarations in the class field above around line
130?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
