emitskevich-blp commented on code in PR #15889:
URL: https://github.com/apache/kafka/pull/15889#discussion_r1617776104
##########
clients/src/test/java/org/apache/kafka/common/metrics/MetricsTest.java:
##########
@@ -475,28 +464,13 @@ public void testPercentiles() {
Metric p50 = this.metrics.metrics().get(metrics.metricName("test.p50",
"grp1"));
Metric p75 = this.metrics.metrics().get(metrics.metricName("test.p75",
"grp1"));
- // record two windows worth of sequential values
- for (int i = 0; i < buckets; i++)
+ // record 1-100 sequential values
+ for (int i = 0; i < buckets; i++) {
sensor.record(i);
-
- assertEquals(25, (Double) p25.metricValue(), 1.0);
- assertEquals(50, (Double) p50.metricValue(), 1.0);
- assertEquals(75, (Double) p75.metricValue(), 1.0);
-
- for (int i = 0; i < buckets; i++)
Review Comment:
Yes, it breaks the test. It relies on the knowledge of internal
samples/windows implementation and effectively tests windows expiration
(`SampledStat`, `SampledStatTest`) instead of testing `Percentiles`. Another
option was to adapt the test for recent changes in windows expiration, but it
seems better to avoid using such knowledge about internals at all. What do you
think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
