Skip to content
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

Merged
merged 6 commits into from
Apr 9, 2025

Conversation

Rancho-7
Copy link
Contributor

@Rancho-7 Rancho-7 commented Apr 5, 2025

jira: https://issues.apache.org/jira/browse/KAFKA-19093

INFO [data-plane Kafka Request Handler on Broker 3000], Resizing
request handler thread pool size from 8 to 10
(kafka.server.KafkaRequestHandlerPool)

it should be "Controller" rather than "Broker"

Reviewers: Ken Huang s7133700@gmail.com, Chia-Ping Tsai
chia7712@gmail.com

@github-actions github-actions bot added triage PRs from the community core Kafka Broker small Small PRs labels Apr 5, 2025
Copy link
Collaborator

@m1a2st m1a2st left a 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 + "], "
Copy link
Collaborator

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?

Copy link
Contributor Author

@Rancho-7 Rancho-7 Apr 5, 2025

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.

@github-actions github-actions bot removed the triage PRs from the community label Apr 6, 2025
Copy link
Collaborator

@m1a2st m1a2st left a 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], "
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

@chia7712 chia7712 left a 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], "

@Rancho-7
Copy link
Contributor Author

Rancho-7 commented Apr 9, 2025

@Rancho-7 could you please following code too?

this.logIdent = s"[Kafka Request Handler $id on ${nodeName.capitalize} $brokerId], "

Removed it.

@chia7712
Copy link
Member

chia7712 commented Apr 9, 2025

the failed test is traced by https://issues.apache.org/jira/browse/KAFKA-7542

@chia7712 chia7712 merged commit 43e22ef into apache:trunk Apr 9, 2025
20 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-approved core Kafka Broker small Small PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants