You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "Nimish2098 (via GitHub)" <gi...@apache.org> on 2026/10/17 10:57:25 UTC
[PR] Fix Flaky StreamAppenderatorTest assertions on Windows #20602 (druid)
Nimish2098 opened a new pull request, #20626:
URL: https://github.com/apache/druid/pull/20626
Fixes #20602
### Description
Fixed a flaky test in `StreamAppenderatorTest` where `persistTimeMillis()` and `mergeTimeMillis()` could occasionally evaluate to `0` in extremely fast execution environments, causing assertion failures. Updated the assertions from `> 0` to `>= 0`.
<!-- Describe your patch: what did you change in code? How did you fix the problem? -->
<!-- If there are several relatively logically separate changes in this PR, create a mini-section for each of them. For example: -->
#### Fixed the bug ...
#### Renamed the class ...
#### Added a forbidden-apis entry ...
<!--
In each section, please describe design decisions made, including:
- Choice of algorithms
- Behavioral aspects. What configuration values are acceptable? How are corner cases and error conditions handled, such as when there are insufficient resources?
- Class organization and design (how the logic is split between classes, inheritance, composition, design patterns)
- Method organization and design (how the logic is split between methods, parameters and return types)
- Naming (class, method, API, configuration, HTTP endpoint, names of emitted metrics)
-->
<!-- It's good to describe an alternative design (or mention an alternative name) for every design (or naming) decision point and compare the alternatives with the designs that you've implemented (or the names you've chosen) to highlight the advantages of the chosen designs and names. -->
<!-- If there was a discussion of the design of the feature implemented in this PR elsewhere (e. g. a "Proposal" issue, any other issue, or a thread in the development mailing list), link to that discussion from this PR description and explain what have changed in your final design compared to your original proposal or the consensus version in the end of the discussion. If something hasn't changed since the original discussion, you can omit a detailed discussion of those aspects of the design here, perhaps apart from brief mentioning for the sake of readability of this PR description. -->
<!-- Some of the aspects mentioned above may be omitted for simple and small changes. -->
<!-- Give your best effort to summarize your changes in a couple of sentences aimed toward Druid users.
If your change doesn't have end user impact, you can skip this section.
For tips about how to write a good release note, see [Release notes](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#release-notes).
-->
<hr>
##### Key changed/added classes in this PR
* `StreamAppenderatorTest`
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [x] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: Track persist and merge time in nanoseconds to fix flaky StreamAppenderatorTest (druid)
Posted by "Nimish2098 (via GitHub)" <gi...@apache.org>.
Nimish2098 closed pull request #20626: fix: Track persist and merge time in nanoseconds to fix flaky StreamAppenderatorTest
URL: https://github.com/apache/druid/pull/20626
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: Track persist and merge time in nanoseconds to fix flaky StreamAppenderatorTest (druid)
Posted by "Nimish2098 (via GitHub)" <gi...@apache.org>.
Nimish2098 commented on code in PR #20626:
URL: https://github.com/apache/druid/pull/20626#discussion_r4321546293
##########
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 pointers. I’ve pushed the nanosecond-based change: persist and merge time are stored in nanoseconds with the millisecond values derived from them, the appenderators record elapsed nanoseconds, the monitor subtracts nanosecond totals before converting, and the test asserts positive nanosecond timing and the millisecond conversion. I also added Stopwatch.nanosElapsed() for this. I’ve updated the title and description. Happy to adjust anything.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: Track persist and merge time in nanoseconds to fix flaky StreamAppenderatorTest (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #20626:
URL: https://github.com/apache/druid/pull/20626#discussion_r4367424346
##########
server/src/main/java/org/apache/druid/segment/realtime/appenderator/StreamAppenderator.java:
##########
@@ -970,8 +971,9 @@ private DataSegmentWithMetadata mergeAndPush(
);
metrics.setMergeCpuTime(JvmUtils.safeGetThreadCpuTime() - startMergeCpuNanos);
- mergeTimeMillis = mergeStopwatch.millisElapsed();
- metrics.setMergeTime(mergeTimeMillis);
+ mergeTimeNanos = mergeStopwatch.nanosElapsed();
+ mergeTimeMillis = TimeUnit.NANOSECONDS.toMillis(mergeTimeNanos);
Review Comment:
this one has not been addressed
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: Track persist and merge time in nanoseconds to fix flaky StreamAppenderatorTest (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 merged PR #20626:
URL: https://github.com/apache/druid/pull/20626
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: Fix Flaky StreamAppenderatorTest assertions on Windows #20602 (druid)
Posted by "Nimish2098 (via GitHub)" <gi...@apache.org>.
Nimish2098 commented on code in PR #20626:
URL: https://github.com/apache/druid/pull/20626#discussion_r4303767289
##########
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 clear breakdown, I'd love to learn and give this refactor a try. Let me know if you have any initial pointers.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: Track persist and merge time in nanoseconds to fix flaky StreamAppenderatorTest (druid)
Posted by "Nimish2098 (via GitHub)" <gi...@apache.org>.
Nimish2098 commented on PR #20626:
URL: https://github.com/apache/druid/pull/20626#issuecomment-6332671329
Thanks, happy to include #20662 in this PR. I’ll push these together:
1. revert the TaskRealtimeMetricsMonitor change so the deltas come from the cumulative millisecond accessors, which fixes the flooring issue from the review
2. inline the merge-time conversion in StreamAppenderator
3. update BatchAppenderatorTest to assert on persistTimeNanos() and mergeTimeNanos()
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: Fix Flaky StreamAppenderatorTest assertions on Windows #20602 (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #20626:
URL: https://github.com/apache/druid/pull/20626#discussion_r4306045548
##########
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 — I reviewed 1 of 1 changed files, and the concern remains actionable. A good starting point is SegmentGenerationMetrics: add nanosecond-backed duration accessors and copy them in snapshot(), then update StreamAppenderator and BatchAppenderator to record elapsed nanoseconds. In TaskRealtimeMetricsMonitor, subtract nanosecond totals before converting the delta to milliseconds for the existing metric. Update the test to assert positive nanosecond timing and the millisecond conversion; apply the same treatment to merge timing, then update the PR title and description.
<!-- 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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: Track persist and merge time in nanoseconds to fix flaky StreamAppenderatorTest (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: Fix Flaky StreamAppenderatorTest assertions on Windows #20602 (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: Track persist and merge time in nanoseconds to fix flaky StreamAppenderatorTest (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #20626:
URL: https://github.com/apache/druid/pull/20626#discussion_r4332667283
##########
server/src/main/java/org/apache/druid/segment/realtime/appenderator/StreamAppenderator.java:
##########
@@ -970,8 +971,9 @@ private DataSegmentWithMetadata mergeAndPush(
);
metrics.setMergeCpuTime(JvmUtils.safeGetThreadCpuTime() - startMergeCpuNanos);
- mergeTimeMillis = mergeStopwatch.millisElapsed();
- metrics.setMergeTime(mergeTimeMillis);
+ mergeTimeNanos = mergeStopwatch.nanosElapsed();
+ mergeTimeMillis = TimeUnit.NANOSECONDS.toMillis(mergeTimeNanos);
Review Comment:
this one is only used for logging, let's inline it where it's used below
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: Track persist and merge time in nanoseconds to fix flaky StreamAppenderatorTest (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on PR #20626:
URL: https://github.com/apache/druid/pull/20626#issuecomment-6331213426
By the way, can you fix the similar problem #20662 in this PR?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: Track persist and merge time in nanoseconds to fix flaky StreamAppenderatorTest (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on PR #20626:
URL: https://github.com/apache/druid/pull/20626#issuecomment-6395056250
Thank you @Nimish2098
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org