ykisana opened a new pull request, #20608:
URL: https://github.com/apache/druid/pull/20608
Fixes #20603.
### Description
`KafkaClusterMetricsTest` is not flaky because of a slow runner. In the
failing run, the only Historical stopped loading segments about 6 seconds into
the test class and did not load another one for the remaining 4.5 minutes. This
was caused by a race in `SegmentLoadDropHandler.processBatch()`.
#### Fixed a lost wakeup in `SegmentLoadDropHandler.processBatch()`
`processBatch()` submits each load/drop asynchronously and reads its status.
Only after that does it register the batch future in `waitingFutures`:
```java
for (DataSegmentChangeRequest cr : changeRequests) {
AtomicReference<SegmentChangeStatus> status = processRequest(cr,
segmentLoadingMode);
if (status.get().getState() != SegmentChangeStatus.State.PENDING) {
isAnyRequestDone = true;
}
...
}
...
synchronized (waitingFutures) {
waitingFutures.add(future);
}
```
When a load finishes, `addSegment()` sets the status and then calls
`resolveWaitingFutures()`, which resolves and clears every future in
`waitingFutures`. If the load finishes after its status was read as `PENDING`
but before the future is added, the callback finds nothing to resolve. The
future then waits for some other request on the server to finish. Nothing else
can finish, because `HttpLoadQueuePeon` keeps only one batch in flight per
server (`mainLoopInProgress`). So the HTTP long-poll returns only when the
async timeout fires, which is
`druid.coordinator.loadqueuepeon.http.hostTimeout` (5 minutes by default).
Until then the server loads and drops nothing.
The fix re-checks the statuses while holding the `waitingFutures` lock, the
same lock `resolveWaitingFutures()` takes. The future is registered only if no
request has finished yet; otherwise it is resolved right away. A load always
updates its status before its callback takes that lock, so one of two things
happens. Either the callback runs after registration and resolves the future,
or the re-check sees the finished status.
`ChangeRequestHistory.getRequestsSince()` already uses this pattern: it checks
and registers under the same lock its producer uses. `resolve()` is still
called outside the `waitingFutures` lock, so the lock order
(`requestStatusesLock` before `waitingFutures`) stays the same.
The response format and semantics of
`/druid-internal/v1/segments/changeRequests` don't change. The future still
resolves as soon as at least one request in the batch is done.
#### Evidence from the failing CI run
From the surefire report and the `ci-debug` jstacks of [job
115925729106](https://github.com/apache/druid/actions/runs/38585343856/job/115925729106):
- The Historical logged `Loading segment` 3 times in the whole class,
compared to 196 in the passing re-run. The last load was at `16:34:46.861`.
- From `16:35:17` the Coordinator logged `Load queue for server
[localhost:8083] ... has [1] segments stuck` for that same segment.
- The jstacks at 16:36, 16:37 and 16:38 show the Historical's
`SegmentLoadDropHandler-normal-0` thread idle. The Coordinator's peon threads
were idle too, and no request failures were logged. So the load had finished,
but its batch response never came back.
- Without handoff, every Kafka task was killed by the 5 s
`completionTimeout` with `No successful task in taskGroup within completion
timeout`: 64 of 64 in the MSQ test and 78 of 80 in the 10k-rows test. The
passing re-run had 0 such failures. As a result, `task/success/count` and
`ingest/handoff/count` were never emitted and the 60 s waits timed out.
Raising the test timeouts, as the issue suggests, would not help. The stall
lasts up to 5 minutes, longer than the 120 s `@Timeout`, and the same stall can
happen on production Historicals.
#### Testing
-
`SegmentLoadDropHandlerTest#testProcessBatchResolvesWhenLoadFinishesBeforeFutureIsRegistered`
forces the interleaving deterministically, with no sleeps. The load executor
holds each load until the next one is submitted, then runs it on the submitting
thread. With a batch `[LOAD segment1, LOAD segment2]`, segment1 finishes after
`processBatch()` has read its status and before the future is registered, and
segment2 never finishes. The test asserts the future is done when
`processBatch()` returns. It fails without the fix (`expected: <true> but was:
<false>`) and passes with it.
- A local real-thread stress run (not included in this PR) used 8 callers,
each with its own handler and a real load executor, plus 16 CPU-hog threads,
for 30 seconds. It counted batch futures that were still unresolved after their
load task, including the callback, had completed:
- without the fix: 14 stranded futures in 311,505 batches;
- with the fix: 0 in 302,882 batches.
- Ran checkstyle and forbidden-apis on `server`.
- Ran the full `server` module test suite (4,108 tests) twice:
- The second run passed.
- In the first run,
`SQLMetadataRuleManagerTest#testAuditEntryCreatedWhenRetentionRulesChanged`
failed. It passes when run alone and as part of the full
`org.apache.druid.metadata` package (546 tests), with and without this change.
It doesn't touch the changed code.
- In the second run,
`CoordinatorSegmentMetadataCacheTest#testSchemaBackfilling` failed once and
then passed on retry. It is also unrelated.
#### Release note
Fixed a race on Historicals and Indexers that serve segments: a segment load
finishing at the wrong moment could leave the Coordinator waiting on that
server for up to `druid.coordinator.loadqueuepeon.http.hostTimeout` (5 minutes
by default). During that time the server loaded and dropped no segments, which
could delay handoff for streaming ingestion.
<hr>
##### Key changed/added classes in this PR
* `SegmentLoadDropHandler`
* `SegmentLoadDropHandlerTest`
<hr>
This PR has:
- [x] been self-reviewed.
- [x] using the [concurrency
checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md).
The bug is a check-then-act race (state used inside a critical section is read
outside of it).
- [x] a release note entry in the PR description.
- [x] 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.
#### Generative AI
Generated by: Claude Opus 5.5
Fix written by and all code manually reviewed by @ykisana.
Tests, PR description, and comments by Claude.
--
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]
