You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "cmccabe (via GitHub)" <gi...@apache.org> on 2023/06/22 03:03:37 UTC
[GitHub] [kafka] cmccabe opened a new pull request, #13900: MINOR: some minor cleanups for process startup
cmccabe opened a new pull request, #13900:
URL: https://github.com/apache/kafka/pull/13900
Move registration of linux-disk-read-bytes, linux-disk-write-bytes metrics into a common function in LinuxIoMetricsCollector.scala. Also do it for isolated controller processes.
Create authorizers using AuthorizerUtils.configureAuthorizer and close them using AuthorizerUtils.closeAuthorizer. This ensures that we configure the authorizers immediately after creating them.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #13900: MINOR: some minor cleanups for process startup
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13900:
URL: https://github.com/apache/kafka/pull/13900#discussion_r1238998513
##########
core/src/main/scala/kafka/metrics/LinuxIoMetricsCollector.scala:
##########
@@ -94,6 +96,22 @@ class LinuxIoMetricsCollector(procRoot: String, val time: Time, val logger: Logg
false
}
}
+
+ def maybeRegisterMetrics(registry: MetricsRegistry): Unit = {
+ def registerGauge(name: String, gauge: Gauge[Long]): Unit = {
+ val metricName = KafkaYammerMetrics.getMetricName(
+ "kafka.server",
Review Comment:
Could we use the constant `Server.MetricsPrefix` here?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a diff in pull request #13900: MINOR: some minor cleanups for process startup
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #13900:
URL: https://github.com/apache/kafka/pull/13900#discussion_r1239118404
##########
core/src/main/scala/kafka/server/BrokerServer.scala:
##########
@@ -75,6 +76,8 @@ class BrokerServer(
// Get raftManager from SharedServer. It will be initialized during startup.
def raftManager: KafkaRaftManager[ApiMessageAndVersion] = sharedServer.raftManager
+ Server.maybeRegisterLinuxMetrics(config, time, logger.underlying)
Review Comment:
Yes, we should remove the registration from KafkaBroker. I'll do that now.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #13900: MINOR: some minor cleanups for process startup
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13900:
URL: https://github.com/apache/kafka/pull/13900#discussion_r1238424518
##########
core/src/main/scala/kafka/metrics/LinuxIoMetricsCollector.scala:
##########
@@ -94,6 +96,22 @@ class LinuxIoMetricsCollector(procRoot: String, val time: Time, val logger: Logg
false
}
}
+
+ def maybeRegisterMetrics(registry: MetricsRegistry): Unit = {
Review Comment:
we should have a similar method for unregister which would be called on shutdown
##########
core/src/main/scala/kafka/metrics/LinuxIoMetricsCollector.scala:
##########
@@ -94,6 +96,22 @@ class LinuxIoMetricsCollector(procRoot: String, val time: Time, val logger: Logg
false
}
}
+
+ def maybeRegisterMetrics(registry: MetricsRegistry): Unit = {
+ def registerGauge(name: String, gauge: Gauge[Long]): Unit = {
+ val metricName = KafkaYammerMetrics.getMetricName(
+ "kafka.server",
Review Comment:
please correct me if I am wrong here, this is a change in metric name. Earlier when we were using `metricsGroup` in `ControllerServer` the name of the metric would have included ControllerServer but now it includes KafkaServer.
Isn't this a breaking change?
##########
core/src/main/scala/kafka/server/BrokerServer.scala:
##########
@@ -75,6 +76,8 @@ class BrokerServer(
// Get raftManager from SharedServer. It will be initialized during startup.
def raftManager: KafkaRaftManager[ApiMessageAndVersion] = sharedServer.raftManager
+ Server.maybeRegisterLinuxMetrics(config, time, logger.underlying)
Review Comment:
This is a new change. Earlier brokers didn't have this metric. Please update the PR title and description to reflect this or start a new PR for this change.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a diff in pull request #13900: MINOR: some minor cleanups for process startup
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #13900:
URL: https://github.com/apache/kafka/pull/13900#discussion_r1238763103
##########
core/src/main/scala/kafka/server/BrokerServer.scala:
##########
@@ -75,6 +76,8 @@ class BrokerServer(
// Get raftManager from SharedServer. It will be initialized during startup.
def raftManager: KafkaRaftManager[ApiMessageAndVersion] = sharedServer.raftManager
+ Server.maybeRegisterLinuxMetrics(config, time, logger.underlying)
Review Comment:
This is not a new change. The metrics are currently registered in `KafkaBroker.scala`. This just moves where they are registered. Here is the existing registration code:
```
private val linuxIoMetricsCollector = new LinuxIoMetricsCollector("/proc", Time.SYSTEM, logger.underlying)
if (linuxIoMetricsCollector.usable()) {
metricsGroup.newGauge("linux-disk-read-bytes", () => linuxIoMetricsCollector.readBytes())
metricsGroup.newGauge("linux-disk-write-bytes", () => linuxIoMetricsCollector.writeBytes())
}
```
These metrics are described in KIP-551, which you can read here: https://cwiki.apache.org/confluence/x/sotSC
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: some minor cleanups for process startup [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #13900: MINOR: some minor cleanups for process startup
URL: https://github.com/apache/kafka/pull/13900
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] github-actions[bot] commented on pull request #13900: MINOR: some minor cleanups for process startup
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13900:
URL: https://github.com/apache/kafka/pull/13900#issuecomment-1728714359
This PR is being marked as stale since it has not had any activity in 90 days. If you would like to keep this PR alive, please ask a committer for review. If the PR has merge conflicts, please update it with the latest from trunk (or appropriate release branch) <p> If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a diff in pull request #13900: MINOR: some minor cleanups for process startup
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #13900:
URL: https://github.com/apache/kafka/pull/13900#discussion_r1238763103
##########
core/src/main/scala/kafka/server/BrokerServer.scala:
##########
@@ -75,6 +76,8 @@ class BrokerServer(
// Get raftManager from SharedServer. It will be initialized during startup.
def raftManager: KafkaRaftManager[ApiMessageAndVersion] = sharedServer.raftManager
+ Server.maybeRegisterLinuxMetrics(config, time, logger.underlying)
Review Comment:
This is not a new change. The metrics are currently registered in `KafkaBroker.scala`. This just moves where they are registered. Here is the existing registration code:
```
private val linuxIoMetricsCollector = new LinuxIoMetricsCollector("/proc", Time.SYSTEM, logger.underlying)
if (linuxIoMetricsCollector.usable()) {
metricsGroup.newGauge("linux-disk-read-bytes", () => linuxIoMetricsCollector.readBytes())
metricsGroup.newGauge("linux-disk-write-bytes", () => linuxIoMetricsCollector.writeBytes())
}```
These metrics are described in KIP-551, which you can read here: https://cwiki.apache.org/confluence/x/sotSC
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #13900: MINOR: some minor cleanups for process startup
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13900:
URL: https://github.com/apache/kafka/pull/13900#discussion_r1238997925
##########
core/src/main/scala/kafka/server/BrokerServer.scala:
##########
@@ -75,6 +76,8 @@ class BrokerServer(
// Get raftManager from SharedServer. It will be initialized during startup.
def raftManager: KafkaRaftManager[ApiMessageAndVersion] = sharedServer.raftManager
+ Server.maybeRegisterLinuxMetrics(config, time, logger.underlying)
Review Comment:
ok. I asked this question because I didn't see any change to `KafkaBroker.scala` in this PR and assumed that we are adding new metrics. I apologize if I am missing something very obvious but I am confused here :(. Shouldn't we remove the registration from KafkaBroker since we are now registering them here?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: some minor cleanups for process startup [kafka]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13900:
URL: https://github.com/apache/kafka/pull/13900#issuecomment-2563280382
This PR has been closed since it has not had any activity in 120 days. If you feel like this
was a mistake, or you would like to continue working on it, please feel free to re-open the
PR and ask for a review.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org