kfaraz commented on code in PR #13238:
URL: https://github.com/apache/druid/pull/13238#discussion_r1007754900
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/stats/TaskRealtimeMetricsMonitorTest.java:
##########
@@ -52,32 +46,31 @@ public void setup()
public void testLastRoundMetricsEmission()
{
FireDepartmentMetrics metrics = new FireDepartmentMetrics();
- RowIngestionMeters rowIngestionMeters = new NoopRowIngestionMeters();
DataSchema schema = new DataSchema("dataSource", null, null, null, null,
null, null, null);
+ // Expect fireDepartment calls twice. Once after start and once when
calling monitorAndStop
EasyMock.expect(fireDepartment.getMetrics()).andReturn(metrics);
EasyMock.expectLastCall().times(2);
EasyMock.expect(fireDepartment.getDataSchema()).andReturn(schema);
EasyMock.expectLastCall().times(2);
EasyMock.replay(fireDepartment);
- TaskRealtimeMetricsMonitor monitor = new
TaskRealtimeMetricsMonitor(fireDepartment, rowIngestionMeters,
ImmutableMap.of());
+ TaskRealtimeMetricsMonitor monitor = new TaskRealtimeMetricsMonitor(
+ fireDepartment, new NoopRowIngestionMeters(), ImmutableMap.of()
+ );
+
+ // No emission since the monitor has not begun
+ Assert.assertFalse(monitor.monitor(emitter));
- Assert.assertFalse(monitor.isStarted());
- boolean zerothRound = monitor.monitor(emitter);
monitor.start();
+ // Monitor has started and a call to fireDepartment is made
Assert.assertTrue(monitor.isStarted());
- boolean firstRound = monitor.monitor(emitter);
- monitor.stop();
- Assert.assertFalse(monitor.isStarted());
- boolean secondRound = monitor.monitor(emitter);
- boolean thirdRound = monitor.monitor(emitter);
+ Assert.assertTrue(monitor.monitor(emitter));
Review Comment:
Much easier to read now!
##########
server/src/main/java/org/apache/druid/segment/realtime/RealtimeMetricsMonitor.java:
##########
@@ -137,6 +114,12 @@ public boolean doMonitor(ServiceEmitter emitter)
emitter.emit(builder.build("ingest/handoff/count",
metrics.handOffCount() - previous.handOffCount()));
emitter.emit(builder.build("ingest/sink/count", metrics.sinkCount()));
emitter.emit(builder.build("ingest/events/messageGap",
metrics.messageGap()));
+
+ long maxSegmentHandoffTime = metrics.maxSegmentHandoffTime();
+ if (maxSegmentHandoffTime >= 0) {
+ emitter.emit(builder.build("ingest/handoff/time",
maxSegmentHandoffTime));
Review Comment:
I wonder if we should just keep the default as 0 rather than -1 and always
emit the `maxSegmentHandoffTime`, even if it is 0. This would also match the
behaviour of the other metrics being emitted here, especially `handoff/count`.
##########
core/src/main/java/org/apache/druid/java/util/metrics/Monitor.java:
##########
@@ -30,6 +30,14 @@
void stop();
+
+ /**
+ * Useful to push a last round of metrics before stopping the monitor
Review Comment:
Nit:
```suggestion
* Emit a last round of metrics using the given emitter and then stop the
monitor.
```
##########
server/src/main/java/org/apache/druid/segment/realtime/appenderator/StreamAppenderatorDriver.java:
##########
@@ -332,6 +332,7 @@ public ListenableFuture<SegmentsAndCommitMetadata>
registerHandoff(SegmentsAndCo
}
log.debug("Register handoff of segments: [%s]", waitingSegmentIdList);
+ long handoffStartTime = System.currentTimeMillis();
Review Comment:
Better to clarify the intent of a final variable rather than relying on it
being "effectively final"
```suggestion
final long handoffStartTime = System.currentTimeMillis();
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
