Skip to content

KAFKA-19207: Move ForwardingManagerMetrics and ForwardingManagerMetricsTest to server module #19574

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

Merged
merged 6 commits into from
May 6, 2025

Conversation

FrankYang0529
Copy link
Member

@FrankYang0529 FrankYang0529 commented Apr 26, 2025

  1. Move ForwardingManagerMetrics and ForwardingManagerMetricsTest to
    server module.
  2. Rewrite them in Java.

Reviewers: Chia-Ping Tsai [email protected]

@github-actions github-actions bot added the core Kafka Broker label Apr 26, 2025
@FrankYang0529 FrankYang0529 requested a review from chia7712 April 26, 2025 13:47
*/
private final LatencyHistogram remoteTimeMsHist;

public final MetricName queueLengthName;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we avoid using public member?

METRIC_GROUP_NAME,
"The current number of RPCs that are waiting in the broker's forwarding manager queue, waiting to be sent to the controller."
);
metrics.addMetric(queueLengthName, new FuncGauge<>(now -> queueLength.get()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is FuncGauge necessary?

@FrankYang0529
Copy link
Member Author

@chia7712 I addressed all comments. Thanks.

@FrankYang0529 FrankYang0529 requested a review from chia7712 May 2, 2025 15:48
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.

@FrankYang0529 thanks for this patch. overall LGTM

public final MetricName latencyP999Name;


public LatencyHistogram(Metrics metrics, String name, String group, long maxLatency) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please use private constructor?

this.latencyP999Name = metrics.metricName(name + ".p999", group);

sensor.add(new Percentiles(
4000,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please add a static constant for it?

@FrankYang0529 FrankYang0529 requested a review from chia7712 May 6, 2025 09:50
@chia7712 chia7712 merged commit 424e725 into apache:trunk May 6, 2025
23 checks passed
@FrankYang0529 FrankYang0529 deleted the KAFKA-19207 branch May 6, 2025 12:50
shmily7829 pushed a commit to shmily7829/kafka that referenced this pull request May 7, 2025
…sTest to server module (apache#19574)

1. Move `ForwardingManagerMetrics` and `ForwardingManagerMetricsTest` to
server module.
2. Rewrite them in Java.

Reviewers: Chia-Ping Tsai <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Kafka Broker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants