DanielLeens opened a new pull request, #12211:
URL: https://github.com/apache/seatunnel/pull/12211
### Purpose of this pull request
Adds one new unit test to `CheckpointCoordinatorTest`, as part of the ongoing
extreme-case E2E/regression coverage initiative for the Zeta engine. This
test closes a coverage gap in the existing regression tests for
apache/seatunnel#10655 ("NPE in
`CheckpointCoordinator.completePendingCheckpoint()`"),
fixed by the merged apache/seatunnel#10705 ("Converge notifyCompleted failure
handling in CheckpointCoordinator").
### Background: the bug and its fix
**Issue #10655**: when `notifyCompleted()` hit an internal exception, it
called `handleCoordinatorError()` -> `cleanPendingCheckpoint()`, which
cleared `pendingCheckpoints`. The original call chain then continued and
called the then-unconditional
`pendingCheckpoints.remove(checkpointId).abortCheckpointTimeoutFutureWhenIsCompleted()`
on the now-already-cleared map, got `null` back, and NPE'd on the chained
call.
**Fix #10705** changed `completePendingCheckpoint()`
(`CheckpointCoordinator.java`, current `dev` HEAD) in two ways, quoted from
the current source:
```java
if (!notifyCompleted(completedCheckpoint)) {
return;
}
PendingCheckpoint pendingCheckpoint =
pendingCheckpoints.remove(checkpointId);
if (pendingCheckpoint != null) {
pendingCheckpoint.abortCheckpointTimeoutFutureWhenIsCompleted();
}
pendingCounter.decrementAndGet();
```
1. `notifyCompleted()` now returns `boolean` (`false` if it caught an
internal exception, after already calling `handleCoordinatorError()`).
All three call sites (`allTaskReady()`, `restoreCoordinator()`,
`completePendingCheckpoint()`) now bail out immediately when it returns
`false`.
2. `pendingCheckpoints.remove(checkpointId)` is now null-checked before the
previously-unconditional `.abortCheckpointTimeoutFutureWhenIsCompleted()`
call is made on it -- defense in depth, independent of (1).
I verified this fix is still intact, unmodified, on current `apache/dev`
(`CheckpointCoordinator.java:1370-1377`; `notifyCompleted()` itself at
`CheckpointCoordinator.java:466-491`; `lock` field at line 168).
### Why this is not a duplicate of apache/seatunnel#10705's own tests
Fix PR #10705 already shipped three white-box unit tests in
`CheckpointCoordinatorTest.java`, still present on current `dev`:
- `testCompletePendingCheckpointShouldReturnEarlyWhenNotifyCompletedFails`
- `testAllTaskReadyShouldNotScheduleCheckpointWhenNotifyCompletedFails`
- `testRestoreCoordinatorShouldNotTriggerCheckpointWhenNotifyCompletedFails`
All three mock `notifyCompleted()` to **return `false`**, which makes
`completePendingCheckpoint()` take the early-return guard in (1)
immediately -- execution never reaches the
`pendingCheckpoints.remove(checkpointId)`
call at all, so the null-check added in (2) is never exercised by any of
them. I confirmed this by grepping the whole current test tree: the string
`abortCheckpointTimeoutFutureWhenIsCompleted` appears nowhere except the two
`src/main` call sites themselves -- no existing test reaches that line.
This PR's new test instead has `notifyCompleted()` **return `true`** (i.e.
it does *not* fail), so `completePendingCheckpoint()` proceeds past guard
(1) into the `pendingCheckpoints.remove(checkpointId)` call -- and asserts
that it still doesn't NPE when that map entry is already gone, exercising
guard (2) specifically.
### Why this is not a duplicate of apache/seatunnel#12199's coverage either
apache/seatunnel#12199
(`CheckpointCoordinatorFailoverIT#testStreamJobFailsAfterCheckpointTriggerDispatchFailure`)
covers a different bug (apache/seatunnel#10442, fixed by
apache/seatunnel#10448) in a
different method: `startTriggerPendingCheckpoint()`'s checkpoint **dispatch**
path (an `IllegalArgumentException` from `queryTaskGroupAddress` when
`ownedSlotProfilesIMap` is missing an entry). It never touches
`notifyCompleted()` or `completePendingCheckpoint()`. This PR's mechanism is
unrelated: the checkpoint **completion/notification** path.
### Trigger mechanism, and why it's reliable without having run it locally
Per this project's Apache SeaTunnel local-verification policy, I have not
run this test locally; GitHub CI on this PR's head is the first execution.
The interleaving this test reproduces is realistic, not contrived:
`completePendingCheckpoint()` is `synchronized` on `this`, but
`cleanPendingCheckpoint()` (invoked from `handleCoordinatorError()`, which
has call sites with no relation to this checkpoint's own `notifyCompleted()`
-- e.g. `reportedTask()`'s async task-status-report handling at
`CheckpointCoordinator.java:309-333`, and other call sites at lines 519,
922, 930, 960, 966, 994 and 1020) synchronizes on the separate `lock` field
(`CheckpointCoordinator.java:168`) instead. The two methods can therefore
genuinely run concurrently on different threads in production: an unrelated
failure elsewhere in the pipeline can clear `pendingCheckpoints` in the
window between this checkpoint's own `notifyCompleted()` returning
successfully and this call reaching its own `remove()`.
Rather than trying to hit that narrow, timing-dependent window with a real
two-thread race (which this coordinator-internal logic doesn't need, and
which I could not have verified deterministically without running it), the
test reproduces the resulting *state* directly: the `notifyCompleted()`
mock clears `pendingCheckpoints` itself (mirroring exactly what a concurrent
`cleanPendingCheckpoint()` does to that map) immediately before returning
`true`. This is the same reflection-based, single-threaded technique
already used (and already merged, already CI-green) by the sibling test
`testCompletePendingCheckpointShouldReturnEarlyWhenNotifyCompletedFails` in
this same file -- only the return value and the resulting code path differ.
It is fully deterministic: no `Thread.sleep`, no `Awaitility` poll, no
timing window to miss.
I chose this unit-level, white-box approach over a full E2E cluster test
(this initiative's usual style, e.g. #12199, #12111, #12110) because this
bug lives entirely in `CheckpointCoordinator`'s own internal error-handling
and map bookkeeping -- reproducing it through a live multi-node cluster
would require forcing a real, precisely-timed race between an unrelated
task failure and a checkpoint's own completion callback, which is far less
reliable to pin down deterministically than driving the coordinator
directly, exactly as PR #10705's own reviewers already accepted for the
sibling tests in this file.
### Does this PR introduce any user-facing change?
No. Test-only; no `src/main` changes.
### How was this patch tested?
- `./mvnw spotless:apply -pl seatunnel-engine/seatunnel-engine-server -nsu
-Dmaven.gitcommitid.skip=true` -- BUILD SUCCESS; spotless reflowed one Javadoc
paragraph, no other changes.
- Per this repo's Apache SeaTunnel local-verification policy, no local
compile/test execution was performed; GitHub CI on this PR's head is the
authoritative verification.
### Check list
- [x] Test-only change, no `src/main` modifications.
- [x] No new dependencies.
- [x] No documentation changes needed (test-only).
🤖 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]
