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-9716: Clarify meaning of compression rate metrics #8664

Merged
merged 2 commits into from
Jun 9, 2020

Conversation

rgroothuijsen
Copy link
Contributor

There is some confusion over the compression rate metrics, as the meaning of the value isn't clearly stated in the metric description. In this case, it was assumed that a higher compression rate value meant better compression. This PR clarifies the meaning of the value, to prevent misunderstandings.

Alternative approaches that were considered were to either change the name of the metric or its implementation, but this would have a negative impact on those who are already making use of this metric.

Committer Checklist (excluded from commit message)

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

Copy link
Contributor

@hachikuji hachikuji 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 the patch. Left a minor suggestion.

@@ -84,7 +84,7 @@ public SenderMetricsRegistry(Metrics metrics) {
this.batchSizeMax = createMetricName("batch-size-max",
"The max number of bytes sent per partition per-request.");
this.compressionRateAvg = createMetricName("compression-rate-avg",
"The average compression rate of record batches.");
"The average compressed-to-uncompressed size ratio of record batches.");
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be a little more verbose, but perhaps we could phrase it like this?

The average compression rate of record batches, defined as the average ratio of the compressed batch size over the uncompressed size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was in fact going for brevity with my approach, but if more detail will mean increased clarity for the user then I'm all for it. With the way you've defined it, I think there can be little question regarding the meaning.

Copy link
Contributor

@hachikuji hachikuji 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 patch!

@hachikuji
Copy link
Contributor

ok to test

@hachikuji
Copy link
Contributor

retest this please

1 similar comment
@hachikuji
Copy link
Contributor

retest this please

@hachikuji hachikuji merged commit 6fa44a3 into apache:trunk Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants