DanielLeens commented on PR #12013:
URL: https://github.com/apache/seatunnel/pull/12013#issuecomment-5476977842
Thanks @dybyte for the +1, and to answer "waiting for CI to pass" — the
`Build` check on the head commit (`76036c652f7c`) is now green (passed, ~9.5h
runtime), so that's cleared.
@SEZ9 thanks for applying the same stability rigor I try to hold this kind
of test-only PR to — since this is specifically a flaky-test fix, I went back
and re-verified each of your four findings against the actual current source
(`CoordinatorServiceTest.java`) rather than the diff alone, applying the same
Stable / Risk present / High risk framework I use for test-stability review.
**Pushing back with evidence — Issue 2's "leak" doesn't happen, by
construction.** The existing `finally` block (unchanged by this PR) already
does exactly what you're asking for:
```java
} finally {
allowFirstScheduleToFinish.countDown();
shutdownCoordinatorIfRunning(coordinatorService);
}
```
This runs unconditionally on every exit path, including the
`Assertions.assertTrue(firstScheduleStarted.await(30, ...))` timeout/failure
path, and it runs *before* `shutdownCoordinatorIfRunning`. But more
importantly: `CountDownLatch` doesn't require the awaiting thread to already be
parked for `countDown()` to take effect. Once `countDown()` drives the count to
zero, the latch stays open permanently — any `await()` call on it, whether it
happens before, during, or after that `countDown()`, returns immediately
without blocking. So there is no ordering window in which the scheduler thread
can "arrive late" and park forever on `allowFirstScheduleToFinish.await()` —
the moment it reaches that line, at any point past the `finally`'s
`countDown()`, it returns right away. I don't think this is a live risk with
the code as written.
**Issues 1 and 3 — agreed these are legitimate, but they're
documentation/hardening notes, not current bugs.** You're right that
`firstScheduleStarted.countDown()` (line 634) only proves the scheduler thread
*entered* the answer, not that it's parked on
`allowFirstScheduleToFinish.await()` (line 635) yet, and that correctness in
that narrow window currently rests on `Thread.interrupt()`'s sticky flag (which
`CountDownLatch.await()` checks on entry per the JDK contract) — this is the
same property I called out in my own review as "harmless... the same
theoretical window existed under the old polling approach too." I agree with
your Issue 3 suggestion: a one-line comment at line 634 documenting that the
latch proves entry-not-parked, and that the interrupt-stickiness is what makes
the immediately-following clear-and-interrupt safe, would help the next person
who touches this method. I'd keep this Low severity rather than blocking, since
it's a "protect against a future refactor"
note rather than something wrong today, but it's cheap and worth adding.
**Correcting a factual point — Issue 4's "unused import" claim isn't
accurate.** `import static org.awaitility.Awaitility.await;` is still very much
alive: I count 54 `await()` call sites across this file, including one still
inside this exact same test method (the post-clear assertion a few lines below
the change: `await().atMost(5, TimeUnit.SECONDS).untilAsserted(() -> {
Assertions.assertFalse(...); Mockito.verify(blockedJobMaster,
Mockito.atLeastOnce()).interrupt(); })`), plus dozens more in sibling tests in
the same class. Spotless won't flag this as dead because it isn't dead. On your
other point in that issue — checking whether sibling tests share the same
verify-vs-running-answer race — I re-checked
`testPendingJobSchedulerIgnoresJobReservedByPreviousScheduler` and
`testPendingJobSchedulerCanAdvanceNextJobWhenPreviousResourceCheckBlocks`: both
already use the same `firstScheduleStarted`/`allowFirstScheduleToFinish`
two-latch handshake (unmodified by this PR, pre-exist
ing), which is exactly why my original review characterized this PR as
"bringing the one outlier test in line with the already-proven pattern" rather
than introducing something new — so I don't think there's a live sibling-test
gap left to migrate. The "extract into a shared helper" idea is still a nice
future hygiene improvement, just not blocking.
**Net stability assessment, unchanged from my original review: Stable.** No
`Thread.sleep`, no new race, the two latches are genuine JDK synchronization
primitives signalled from the exact code path under test, and — per the
CountDownLatch semantics above — no ordering of `countDown()`/`await()` calls
across the two latches used here can produce a hang. Issues 1 and 3 are worth a
follow-up comment for future maintainers; Issue 2 and the import half of Issue
4 don't hold up against the current source. Thanks again for the rigorous
second look — this is exactly the kind of check a test-stabilization PR should
get.
--
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]
