amaechler opened a new pull request, #20408:
URL: https://github.com/apache/druid/pull/20408
### Description
`DruidCoordinator.getStatusOfDuties()` reads `dutiesRunnables` without
holding `lock`, while `becomeLeader()` and `stopAllDutyGroups()` mutate the
same `ArrayList` under `lock`. A caller of `/druid/coordinator/v1/duties` that
races with a leadership change can therefore observe a half-cleared list and
hit a `NullPointerException` in the stream.
The same race makes `DruidCoordinatorTest` flaky. Three tests
(`testCompactSegmentsDutyWhenCustomDutyGroupEmpty` and the two
`testInitializeCompactSegmentsDuty…` tests) call `start()` and immediately read
`getStatusOfDuties()`. They stub
`segmentsMetadataManager.isPollingDatabasePeriodically()` but not
`serverInventoryView.isStarted()`, so the mock answers `false`. With the 1 ms
start delay in the test config, the first duty run takes the "Inventory view
not initialized yet" branch and calls `stopBeingLeader()`, which clears
`dutiesRunnables` on the executor thread. Depending on timing the test thread
sees 4 groups, 0 groups, or:
```
java.lang.NullPointerException: Cannot read field "dutyGroup" because "r" is
null
at DruidCoordinator.lambda$getStatusOfDuties$0(DruidCoordinator.java:355)
```
Inserting `Thread.sleep(50)` before the `getStatusOfDuties()` call makes the
tests fail deterministically with `expected: <4> but was: <0>`.
#### Guarded `dutiesRunnables` with `lock`
Annotated the field with `@GuardedBy("lock")` and wrapped the read in
`getStatusOfDuties()` in `synchronized (lock)`. Every other access already runs
under `lock`, so this is the only call site that changes. Error Prone's
`GuardedBy` checker (enabled in the `strict` profile) now enforces the
invariant at compile time.
#### Stubbed `serverInventoryView.isStarted()` in the three flaky tests
The three tests now stub `serverInventoryView.isStarted()` to `true`, as
every other test in the class already does, so the coordinator keeps leadership
for the duration of the test.
<hr>
##### Key changed/added classes in this PR
* `DruidCoordinator`
* `DruidCoordinatorTest`
<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)
- [x] added or updated unit tests.
--
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]
