junrao commented on code in PR #15889:
URL: https://github.com/apache/kafka/pull/15889#discussion_r1617667802
##########
clients/src/test/java/org/apache/kafka/common/metrics/stats/FrequenciesTest.java:
##########
@@ -102,58 +110,69 @@ public void testBooleanFrequencies() {
}
@Test
- public void testUseWithMetrics() {
- MetricName name1 = name("1");
- MetricName name2 = name("2");
- MetricName name3 = name("3");
- MetricName name4 = name("4");
- Frequencies frequencies = new Frequencies(4, 1.0, 4.0,
- new Frequency(name1, 1.0),
- new Frequency(name2, 2.0),
- new Frequency(name3, 3.0),
- new Frequency(name4, 4.0));
+ public void testWithMetricsStrategy1() {
+ Frequencies frequencies = new Frequencies(4, 1.0, 4.0, freq("1", 1.0),
+ freq("2", 2.0), freq("3", 3.0), freq("4", 4.0));
Sensor sensor = metrics.sensor("test", config);
sensor.add(frequencies);
- Metric metric1 = this.metrics.metrics().get(name1);
- Metric metric2 = this.metrics.metrics().get(name2);
- Metric metric3 = this.metrics.metrics().get(name3);
- Metric metric4 = this.metrics.metrics().get(name4);
- // Record 2 windows worth of values
- for (int i = 0; i != 100; ++i) {
+ // Record 100 events uniformly between all buckets
+ for (int i = 0; i < 100; ++i) {
frequencies.record(config, i % 4 + 1, time.milliseconds());
}
- assertEquals(0.25, (Double) metric1.metricValue(), DELTA);
- assertEquals(0.25, (Double) metric2.metricValue(), DELTA);
- assertEquals(0.25, (Double) metric3.metricValue(), DELTA);
- assertEquals(0.25, (Double) metric4.metricValue(), DELTA);
+ assertEquals(0.25, metricValueFor("1"), DELTA);
+ assertEquals(0.25, metricValueFor("2"), DELTA);
+ assertEquals(0.25, metricValueFor("3"), DELTA);
+ assertEquals(0.25, metricValueFor("4"), DELTA);
+ }
+
+ @Test
+ public void testWithMetricsStrategy2() {
+ Frequencies frequencies = new Frequencies(4, 1.0, 4.0, freq("1", 1.0),
+ freq("2", 2.0), freq("3", 3.0), freq("4", 4.0));
+ Sensor sensor = metrics.sensor("test", config);
+ sensor.add(frequencies);
+
+ // Record 100 events half-half between 1st and 2nd buckets
+ for (int i = 0; i < 100; ++i) {
+ frequencies.record(config, i % 2 + 1, time.milliseconds());
+ }
+ assertEquals(0.50, metricValueFor("1"), DELTA);
+ assertEquals(0.50, metricValueFor("2"), DELTA);
+ assertEquals(0.00, metricValueFor("3"), DELTA);
+ assertEquals(0.00, metricValueFor("4"), DELTA);
+ }
- // Record 2 windows worth of values
- for (int i = 0; i != 100; ++i) {
+ @Test
+ public void testWithMetricsStrategy3() {
+ Frequencies frequencies = new Frequencies(4, 1.0, 4.0, freq("1", 1.0),
+ freq("2", 2.0), freq("3", 3.0), freq("4", 4.0));
+ Sensor sensor = metrics.sensor("test", config);
+ sensor.add(frequencies);
+
+ // Record 50 events half-half between 1st and 2nd buckets
+ for (int i = 0; i < 50; ++i) {
frequencies.record(config, i % 2 + 1, time.milliseconds());
}
- assertEquals(0.50, (Double) metric1.metricValue(), DELTA);
- assertEquals(0.50, (Double) metric2.metricValue(), DELTA);
- assertEquals(0.00, (Double) metric3.metricValue(), DELTA);
- assertEquals(0.00, (Double) metric4.metricValue(), DELTA);
-
- // Record 1 window worth of values to overlap with the last window
- // that is half 1.0 and half 2.0
- for (int i = 0; i != 50; ++i) {
+ // Record 50 events to 4th bucket
+ for (int i = 0; i < 50; ++i) {
frequencies.record(config, 4.0, time.milliseconds());
}
- assertEquals(0.25, (Double) metric1.metricValue(), DELTA);
- assertEquals(0.25, (Double) metric2.metricValue(), DELTA);
- assertEquals(0.00, (Double) metric3.metricValue(), DELTA);
- assertEquals(0.50, (Double) metric4.metricValue(), DELTA);
+ assertEquals(0.25, metricValueFor("1"), DELTA);
+ assertEquals(0.25, metricValueFor("2"), DELTA);
+ assertEquals(0.00, metricValueFor("3"), DELTA);
+ assertEquals(0.50, metricValueFor("4"), DELTA);
}
- protected MetricName name(String metricName) {
+ private MetricName name(String metricName) {
Review Comment:
name => toMetricName ?
##########
clients/src/test/java/org/apache/kafka/common/metrics/stats/FrequenciesTest.java:
##########
@@ -73,14 +72,14 @@ public void testMoreFrequencyParametersThanBuckets() {
}
@Test
- public void testBooleanFrequencies() {
+ public void testBooleanFrequenciesStrategy1() {
Review Comment:
Just curious. It seems that we just break the existing tests into smaller
ones. Did we change any logic for the tests to pass?
##########
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++) {
Review Comment:
In some other places in this class, we don't use {} for single line
statements. Should we be consistent?
##########
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:
Why do we remove this code? Does it break the test?
##########
clients/src/test/java/org/apache/kafka/common/metrics/stats/FrequenciesTest.java:
##########
@@ -89,8 +88,17 @@ public void testBooleanFrequencies() {
}
assertEquals(0.25, falseMetric.stat().measure(config,
time.milliseconds()), DELTA);
assertEquals(0.75, trueMetric.stat().measure(config,
time.milliseconds()), DELTA);
+ }
- // Record 2 more windows worth of values
+ @Test
+ public void testBooleanFrequenciesStrategy2() {
Review Comment:
Does `testBooleanFrequenciesStrategy2` cover anything different from
`testBooleanFrequenciesStrategy1`?
--
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
