-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
KAFKA-19093: Change the "Handler on Broker" to "Handler on Controller" for controller server #19384
Conversation
…" for controller server
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.
Thanks for this patch, left a comment
@@ -208,7 +208,7 @@ class KafkaRequestHandlerPool( | |||
/* a meter to track the average free capacity of the request handlers */ | |||
private val aggregateIdleMeter = metricsGroup.newMeter(requestHandlerAvgIdleMetricName, "percent", TimeUnit.NANOSECONDS) | |||
|
|||
this.logIdent = "[" + logAndThreadNamePrefix + " Kafka Request Handler on Broker " + brokerId + "], " | |||
this.logIdent = "[" + logAndThreadNamePrefix + " Kafka Request Handler on Controller " + brokerId + "], " |
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.
I think that we should use ${nodeName.capitalize}
instead of hard core, WDYT?
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.
You're right,I completely agree with your suggestion.Thanks for catching this! Fixed 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.
Thanks for this patch, LGTM
@@ -208,7 +208,7 @@ class KafkaRequestHandlerPool( | |||
/* a meter to track the average free capacity of the request handlers */ | |||
private val aggregateIdleMeter = metricsGroup.newMeter(requestHandlerAvgIdleMetricName, "percent", TimeUnit.NANOSECONDS) | |||
|
|||
this.logIdent = "[" + logAndThreadNamePrefix + " Kafka Request Handler on Broker " + brokerId + "], " | |||
this.logIdent = s"[$logAndThreadNamePrefix Kafka Request Handler on ${nodeName.capitalize} $brokerId], " |
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.
Maybe we can remove the ",". it is a bit weird to me, WDYT?
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.
Thanks for the suggestion! Removed 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.
@Rancho-7 could you please following code too?
this.logIdent = s"[Kafka Request Handler $id on ${nodeName.capitalize} $brokerId], " |
Removed it. |
the failed test is traced by https://issues.apache.org/jira/browse/KAFKA-7542 |
jira: https://issues.apache.org/jira/browse/KAFKA-19093
it should be "Controller" rather than "Broker"
Reviewers: Ken Huang s7133700@gmail.com, Chia-Ping Tsai
chia7712@gmail.com