jsancio commented on code in PR #20422:
URL: https://github.com/apache/kafka/pull/20422#discussion_r2363247573
##########
metadata/src/main/java/org/apache/kafka/controller/QuorumController.java:
##########
@@ -406,7 +406,8 @@ public QuorumController build() throws Exception {
KafkaEventQueue queue = null;
try {
- queue = new KafkaEventQueue(time, logContext,
threadNamePrefix);
+ queue = new KafkaEventQueue(time, logContext,
threadNamePrefix, EventQueue.VoidEvent.INSTANCE,
controllerMetrics::updateIdleTime);
+
Review Comment:
Let's fix this formatting. E.g.
```java
queue = new KafkaEventQueue(
time,
logContext,
threadNamePrefix,
EventQueue.VoidEvent.INSTANCE,
controllerMetrics::updateIdleTime
);
```
##########
server-common/src/main/java/org/apache/kafka/queue/KafkaEventQueue.java:
##########
@@ -278,22 +279,21 @@ private void handleEvents() {
remove(toRun);
continue;
}
- if (awaitNs == Long.MAX_VALUE) {
- try {
+
+ long startIdleMs = time.milliseconds();
+ try {
+ if (awaitNs == Long.MAX_VALUE) {
cond.await();
- } catch (InterruptedException e) {
- log.warn("Interrupted while waiting for a new
event. " +
- "Shutting down event queue");
- interrupted = true;
- }
- } else {
- try {
+ } else {
cond.awaitNanos(awaitNs);
- } catch (InterruptedException e) {
- log.warn("Interrupted while waiting for a deferred
event. " +
- "Shutting down event queue");
- interrupted = true;
}
+ } catch (InterruptedException e) {
+
+ log.warn("Interrupted while waiting for a {} event. " +
+ "Shutting down event queue", (awaitNs ==
Long.MAX_VALUE) ? "new" : "deferred");
Review Comment:
Extra newline and odd formatting. E.g.
```java
log.warn(
"Interrupted while waiting for a {} event.
Shutting down event queue",
(awaitNs == Long.MAX_VALUE) ? "new" : "deferred"
);
```
##########
server-common/src/main/java/org/apache/kafka/queue/KafkaEventQueue.java:
##########
@@ -474,6 +479,7 @@ public KafkaEventQueue(
this.eventHandler, false);
this.shuttingDown = false;
this.interrupted = false;
+ this.idleTimeCallback = idleTimeCallback != null ? idleTimeCallback :
__ -> { };
Review Comment:
I suggest not using `null` in public interfaces. Please use `Optional<T>` if
a parameter is optional.
FYI:
https://www.infoq.com/presentations/Null-References-The-Billion-Dollar-Mistake-Tony-Hoare/
##########
metadata/src/main/java/org/apache/kafka/controller/metrics/QuorumControllerMetrics.java:
##########
@@ -157,6 +165,16 @@ public Long value() {
return newActiveControllers();
}
}));
+ registry.ifPresent(r -> r.newGauge(AVERAGE_IDLE_RATIO, new
Gauge<Double>() {
+ @Override
+ public Double value() {
+ return avgIdleTimeRatio.measure(METRIC_CONFIG,
time.milliseconds());
+ }
+ }));
+ }
+
+ public void updateIdleTime(long idleDurationMs) {
+ avgIdleTimeRatio.record(METRIC_CONFIG, (double) idleDurationMs,
time.milliseconds());
Review Comment:
Why create a METRIC_CONFIG to have it ignored by TimeRatio? The issue is
that TimeRatio implements an API specific for Kafka Metrics. One solution is to
add a `measure` and `record` method to TimeRatio that doesn't require this
metrics configuration.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
