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

MINOR: prevent exception from HdrHistogram #18674

Merged
merged 4 commits into from
Jan 29, 2025
Merged

MINOR: prevent exception from HdrHistogram #18674

merged 4 commits into from
Jan 29, 2025

Conversation

jeffkbkim
Copy link
Contributor

HdrHistogram can throw an exception if the recorded value is greater than a configured limit. Expand the ceiling from per-metric to all invocations. Using existing test.

Committer Checklist (excluded from commit message)

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

@jeffkbkim jeffkbkim added the KIP-848 The Next Generation of the Consumer Rebalance Protocol label Jan 22, 2025
@github-actions github-actions bot added triage PRs from the community small Small PRs labels Jan 22, 2025
@@ -125,6 +127,6 @@ public List<NamedMeasurable> stats() {

@Override
public void record(MetricConfig config, double value, long timeMs) {
hdrHistogram.record((long) value);
hdrHistogram.record(Math.min(highestTrackableValue, (long) value));
Copy link
Member

Choose a reason for hiding this comment

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

Should we add a unit test for this change in KafkaMetricHistogramTest
?

Copy link
Member

Choose a reason for hiding this comment

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

out of curiosity, HdrHistogram is the wrap of HdrHistogram API, so maybe we can move this protection into HdrHistogram?

@github-actions github-actions bot removed the triage PRs from the community label Jan 24, 2025
@jeffkbkim
Copy link
Contributor Author

@dajac @chia7712 this is ready for another round of review

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.

LGTM

@chia7712
Copy link
Member

#18735 is handling the flaky testAdminClientApisAuthenticationFailure

@jeffkbkim jeffkbkim merged commit 048dfef into trunk Jan 29, 2025
7 of 9 checks passed
@jeffkbkim jeffkbkim deleted the minor-kafka-18610 branch January 29, 2025 16:34
jeffkbkim added a commit that referenced this pull request Jan 30, 2025
HdrHistogram can throw an exception if the recorded value is greater than a configured limit. Expand the ceiling from per-metric to all invocations.

Reviewers: David Jacot <[email protected]>, Chia-Ping Tsai <[email protected]>
@jeffkbkim
Copy link
Contributor Author

picked to 4.0

ijuma added a commit to ijuma/kafka that referenced this pull request Jan 30, 2025
…ibrdkafka-compressed-produce-fails

* apache-github/trunk:
  MINOR: prevent exception from HdrHistogram (apache#18674)
  KAFKA-18653: Fix mocks and potential thread leak issues causing silent RejectedExecutionException in share group broker tests (apache#18725)
  KAFKA-18646: Null records in fetch response breaks librdkafka (apache#18726)
  KAFKA-18619: New consumer topic metadata events should set requireMetadata flag (apache#18668)
  KAFKA-18488: Improve KafkaShareConsumerTest (apache#18728)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
KIP-848 The Next Generation of the Consumer Rebalance Protocol small Small PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants