suraj-goel opened a new pull request, #20519:
URL: https://github.com/apache/druid/pull/20519
Fixes #20518.
### Description
`EmbeddedKafkaSupervisorTest.test_runKafkaSupervisorWithHeaderFiltering`
(added in #18525) failed its first attempt in 6 of 7 master runs. Each time, it
timed out waiting for `ingest/handoff/count` to reach 4. It fails only when it
runs after the other tests in the class. The surefire retry runs it alone, so
the retry passed.
#### Root cause
The task does read all 10 records. The race is between the test suspending
the supervisor and the task's own checkpoint:
1. With `maxRowsPerSegment = 1`, the task requests a checkpoint right after
it reads its batch.
2. The test suspended the supervisor as soon as the broker emitted any
metric for the datasource. In a warm cluster, that happens within milliseconds
of the first segment allocation. The suspend therefore replaces the supervisor
while the checkpoint request is in flight.
3. The overlord rejects the checkpoint (`supervisor could not be found`).
The task fails with `Checkpoint request ... failed, dying`, and nothing is
published.
The spec also used the default `startDelay` (`PT5S`) and `period` (`PT30S`).
The supervisor adds a newly created task to its task group only on its next
run. Until then, a checkpoint request does nothing (`New checkpoint is null`)
and leaves the task paused. The test depended on the suspend for publishing:
the suspended supervisor's first run, 5 seconds later, set the end offsets.
The fix suggested in the issue (wait for `ingest/events/processed` >= 4
before suspending) makes the race less likely but does not remove it.
`processed` is incremented during parsing, a few milliseconds before the
checkpoint request is sent, and the metric is emitted every 100 ms.
#### Fix
- The header-filter supervisor spec is now built from
`newKafkaSupervisor()`, like the other tests in the class, with only the header
filter added. The removed builder was identical apart from the timing settings:
500 ms task duration, 500 ms run period, and 10 ms start delay.
- The test now waits for handoff without suspending the supervisor. The
short task duration publishes the segments. The existing `@AfterEach` teardown
then suspends the supervisor and cancels the remaining tasks.
Suspending a supervisor while a task's checkpoint is in flight fails that
task. The failure is recoverable: the task's offsets were not committed, so
resumed tasks re-read the data. This PR does not change that behavior.
#### Testing
Ran locally on JDK 25 with `-Dsurefire.rerunFailingTestsCount=0`:
| Version | Full class | Test alone |
|---|---|---|
| Before | 0/4 passed (same timeout at line 220 as CI) | 5/5 passed |
| Issue's suggested fix | 6/6 passed (race still possible) | not run |
| This PR | 8/8 passed | 3/3 passed |
With this change, handoff completes about 1.6 seconds after the supervisor
starts, compared with about 5.5 seconds after the suspend before.
<hr>
##### Key changed/added classes in this PR
* `EmbeddedKafkaSupervisorTest`
<hr>
This PR has:
- [x] been self-reviewed.
- [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.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
