FrankChen021 commented on code in PR #20626:
URL: https://github.com/apache/druid/pull/20626#discussion_r4324993776
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/stats/TaskRealtimeMetricsMonitor.java:
##########
@@ -127,7 +128,9 @@ public boolean doMonitor(ServiceEmitter emitter)
emitter.emit(builder.setMetric("ingest/rows/output", metrics.rowOutput() -
previousSegmentGenerationMetrics.rowOutput()));
emitter.emit(builder.setMetric("ingest/persists/count",
metrics.numPersists() - previousSegmentGenerationMetrics.numPersists()));
- emitter.emit(builder.setMetric("ingest/persists/time",
metrics.persistTimeMillis() -
previousSegmentGenerationMetrics.persistTimeMillis()));
+ // Subtract in nanoseconds and convert once, to avoid accumulating a
rounding error from converting each total first.
+ final long persistTimeDeltaNanos = metrics.persistTimeNanos() -
previousSegmentGenerationMetrics.persistTimeNanos();
+ emitter.emit(builder.setMetric("ingest/persists/time",
TimeUnit.NANOSECONDS.toMillis(persistTimeDeltaNanos)));
Review Comment:
[P2] Preserve sub-millisecond time across monitor ticks
**Finding:** The monitor floors each nanosecond delta to milliseconds on
every tick. If a task accumulates, for example, 0.6 ms of persist work between
successive monitor calls, each emission is 0 even though the cumulative
millisecond value eventually reaches 1; repeated sub-millisecond intervals can
therefore permanently undercount ingest/persists/time. The identical conversion
at line 144 has the same issue for merge time.
**Suggestion:** Carry the nanosecond remainder across monitor ticks or
derive deltas from the cumulative millisecond accessors now backed by
nanoseconds, applying the same treatment to persist and merge metrics.
##########
server/src/test/java/org/apache/druid/segment/realtime/appenderator/StreamAppenderatorTest.java:
##########
@@ -189,10 +189,10 @@ public void testSimpleIngestion() throws Exception
SegmentGenerationMetrics segmentGenerationMetrics = tester.getMetrics();
Assertions.assertEquals(2, segmentGenerationMetrics.numPersists());
Assertions.assertEquals(3, segmentGenerationMetrics.rowOutput());
- Assertions.assertTrue(segmentGenerationMetrics.persistTimeMillis() > 0);
+ Assertions.assertTrue(segmentGenerationMetrics.persistTimeMillis() >= 0);
Review Comment:
Follow-up assessment: I reviewed 6 of 6 changed files at head
9b1856a5a17943e2f2d755ab3a81ca9969ac2fdd. The requested nanosecond-backed
storage, appenderator updates, snapshot copying, and test assertions address
the original flakiness. One issue remains: `TaskRealtimeMetricsMonitor` floors
each per-tick nanosecond delta, so repeated sub-millisecond persist or merge
work can still be permanently undercounted. Please preserve the remainder or
derive the delta from cumulative millisecond values.
<!-- mergelens:review -->
--
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]
