Skip to content

KAFKA-17069: Remote copy throttle metrics#16086

Merged
showuon merged 1 commit intoapache:trunkfrom
abhijeetk88:KAFKA-15434_copy_quota_metrics
Jul 4, 2024
Merged

KAFKA-17069: Remote copy throttle metrics#16086
showuon merged 1 commit intoapache:trunkfrom
abhijeetk88:KAFKA-15434_copy_quota_metrics

Conversation

@abhijeetk88
Copy link
Copy Markdown
Contributor

@abhijeetk88 abhijeetk88 commented May 27, 2024

As part of KIP-956, we have added quota for remote copies to remote storage. In this PR, we are adding the following metrics for remote copy throttling.

Metric Description
remote-copy-throttle-time-avg The average time in millis remote copies was throttled by a broker
remote-copy-throttle-time-max The max time in millis remote copies was throttled by a broker

Added unit test for the metrics.

I will have a follow-up PR to add these metrics to Kafka documentation.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@abhijeetk88 abhijeetk88 marked this pull request as draft May 27, 2024 06:07
@gharris1727 gharris1727 added kip Requires or implements a KIP tiered-storage Related to the Tiered Storage feature labels May 28, 2024
@abhijeetk88 abhijeetk88 force-pushed the KAFKA-15434_copy_quota_metrics branch from 6b2c9b0 to 69a2e93 Compare June 9, 2024 08:04
@abhijeetk88 abhijeetk88 marked this pull request as ready for review June 9, 2024 08:08
@satishd satishd requested review from kamalcph, satishd and showuon June 10, 2024 10:12
@abhijeetk88 abhijeetk88 force-pushed the KAFKA-15434_copy_quota_metrics branch from 69a2e93 to 4d5de93 Compare June 12, 2024 06:49
@kamalcph
Copy link
Copy Markdown
Contributor

@abhijeetk88

Can you resolve the conflicts on this PR?

@abhijeetk88 abhijeetk88 force-pushed the KAFKA-15434_copy_quota_metrics branch from 4d5de93 to 6652e29 Compare June 12, 2024 14:48
Copy link
Copy Markdown
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

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

Left some comments. Like the other PR, could we add some tests to verify the real metrics are updated correctly? Something like RemoteLogManagerTest#assertCopyExpectedLogSegmentsToRemote or ReplicaManagerTest#testRemoteLogReaderMetrics test. Thanks.

Comment thread core/src/main/java/kafka/log/remote/RemoteLogManager.java Outdated
@abhijeetk88 abhijeetk88 force-pushed the KAFKA-15434_copy_quota_metrics branch from 6652e29 to 90c68a5 Compare July 2, 2024 11:30
@abhijeetk88 abhijeetk88 force-pushed the KAFKA-15434_copy_quota_metrics branch from 90c68a5 to f99c139 Compare July 3, 2024 08:53
@abhijeetk88 abhijeetk88 changed the title KAFKA-15265: Remote copy throttle metrics KAFKA-17069: Remote copy throttle metrics Jul 3, 2024
@abhijeetk88
Copy link
Copy Markdown
Contributor Author

@showuon I have updated the PR. Please take another look.

Copy link
Copy Markdown
Member

@satishd satishd left a comment

Choose a reason for hiding this comment

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

Thanks @abhijeetk88 for the PR. LGTM.

Copy link
Copy Markdown
Member

@showuon showuon left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the PR!

@showuon
Copy link
Copy Markdown
Member

showuon commented Jul 4, 2024

Failed tests are unrelated.

@showuon showuon merged commit 641469e into apache:trunk Jul 4, 2024
abhi-ksolves pushed a commit to ksolves/kafka that referenced this pull request Jul 31, 2024
As part of KIP-956, we have added quota for remote copies to remote storage. In this PR, we are adding the following metrics for remote copy throttling.
1. remote-copy-throttle-time-avg 	The average time in millis remote copies was throttled by a broker
2. remote-copy-throttle-time-max 	The max time in millis remote copies was throttled by a broker

Added unit test for the metrics.

Reviewers: Kamal Chandraprakash<kamal.chandraprakash@gmail.com>, Luke Chen <showuon@gmail.com>, Satish Duggana <satishd@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kip Requires or implements a KIP tiered-storage Related to the Tiered Storage feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants