FrankChen021 commented on code in PR #20626:
URL: https://github.com/apache/druid/pull/20626#discussion_r4302972163
##########
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:
Thanks for the change. Updating the condition to use >= solves the flakiness
problem, but it changes the semantics of the test case that `persistTimeMillis`
should be updated.
The root cause here is: incrementPersistTimeMillis adds a millisecond value
rounded down on every persist, so persists under 1 ms count as 0.
A better fix is(although the change is a little bit broader):
- SegmentGenerationMetrics: store persist time in nanoseconds, add
persistTimeNanos(), and have persistTimeMillis() convert from nanoseconds.
- StreamAppenderator / BatchAppenderator: pass nanoseconds.
- TaskRealtimeMetricsMonitor: subtract in nanoseconds, then convert.
- Test: assert persistTimeNanos() > 0 and metrics.persistTimeMillis() ==
metrics.persistTimeNanos() / 1000_000
PR title and description should also be updated to reflect the change
--
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]
