soarez commented on code in PR #15622:
URL: https://github.com/apache/kafka/pull/15622#discussion_r1543829135
##########
metadata/src/main/java/org/apache/kafka/controller/metrics/SlowEventsLogger.java:
##########
@@ -0,0 +1,58 @@
+package org.apache.kafka.controller.metrics;
+
+import org.apache.kafka.common.utils.Time;
+import org.apache.kafka.common.utils.Timer;
+import org.slf4j.Logger;
+
+import java.util.function.Supplier;
+
+import static java.util.concurrent.TimeUnit.MICROSECONDS;
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import static java.util.concurrent.TimeUnit.NANOSECONDS;
+
+public class SlowEventsLogger {
+ /**
+ * Don't report any p99 events below this threshold. This prevents the
controller from reporting p99 event
+ * times in the idle case.
+ */
+ private static final int MIN_SLOW_EVENT_TIME_MS = 100;
+
+ /**
+ * Calculating the p99 from the histogram consumes some resources, so only
update it every so often.
+ */
+ private static final int P99_REFRESH_INTERVAL_MS = 30000;
+
+ private final Supplier<Double> p99Supplier;
+ private final Logger log;
+ private final Time time;
+ private double p99;
+ private Timer percentileUpdateTimer;
+
+ public SlowEventsLogger(
+ Supplier<Double> p99Supplier,
+ Logger log,
+ Time time
+ ) {
+ this.p99Supplier = p99Supplier;
+ this.log = log;
+ this.time = time;
+ this.percentileUpdateTimer = time.timer(P99_REFRESH_INTERVAL_MS);
+ }
+
+ public void maybeLog(String name, long durationNs) {
+ long durationMs = MILLISECONDS.convert(durationNs, NANOSECONDS);
+ if (durationMs > MIN_SLOW_EVENT_TIME_MS && durationMs > p99) {
Review Comment:
It seems possible that `p99 = p99Supplier.get();` may not have run yet at
this point. Should `p99` have an explicit default value?
##########
metadata/src/main/java/org/apache/kafka/controller/metrics/QuorumControllerMetrics.java:
##########
@@ -28,6 +28,8 @@
import java.util.Optional;
import java.util.concurrent.atomic.AtomicLong;
import java.util.function.Consumer;
+import java.util.function.Function;
+import java.util.function.Supplier;
Review Comment:
Is `Supplier` needed here?
--
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
