[Fix][Zeta] Fix local mode can not finish#9549
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses an issue where local mode does not finish by improving interruption handling in the pending job scheduler thread and adding a test to verify thread lifecycle.
- Add specific
InterruptedExceptioncatch inCoordinatorServiceto avoid swallowing interrupts. - Enhance
CoordinatorServiceTestto assert that the scheduler thread starts and stops correctly. - Refactor test assertions for clarity.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| CoordinatorService.java | Handle InterruptedException explicitly in the scheduler thread loop. |
| CoordinatorServiceTest.java | Capture initial thread count, assert it’s >0, then verify count decrements after shutdown; changed assertions to assertEquals. |
Comments suppressed due to low confidence (1)
seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/CoordinatorServiceTest.java:202
- [nitpick] Rename
scheduleRunnerThreadCounttoinitialSchedulerThreadCountfor clearer intent in the setup phase of the test.
int scheduleRunnerThreadCount =
| try { | ||
| pendingJobSchedule(); | ||
| } catch (InterruptedException interrupted) { | ||
| throw new RuntimeException(interrupted); |
There was a problem hiding this comment.
Instead of throwing a RuntimeException on interrupt, consider restoring the thread's interrupted status with Thread.currentThread().interrupt() and breaking the loop to allow graceful shutdown.
| throw new RuntimeException(interrupted); | |
| Thread.currentThread().interrupt(); | |
| break; |
| Assertions.assertEquals( | ||
| scheduleRunnerThreadCount - 1, | ||
| Thread.getAllStackTraces().keySet().stream() | ||
| .filter( | ||
| thread -> | ||
| thread.getName().startsWith("pending-job-schedule-runner")) | ||
| .count()); |
There was a problem hiding this comment.
[nitpick] This direct count check may be flaky. Use an awaitility or join on the scheduler thread with a timeout to ensure it has stopped before asserting the thread count.
| Assertions.assertEquals( | |
| scheduleRunnerThreadCount - 1, | |
| Thread.getAllStackTraces().keySet().stream() | |
| .filter( | |
| thread -> | |
| thread.getName().startsWith("pending-job-schedule-runner")) | |
| .count()); | |
| await().atMost(10, TimeUnit.SECONDS) | |
| .untilAsserted(() -> Assertions.assertEquals( | |
| scheduleRunnerThreadCount - 1, | |
| Thread.getAllStackTraces().keySet().stream() | |
| .filter( | |
| thread -> | |
| thread.getName().startsWith("pending-job-schedule-runner")) | |
| .count())); |
Purpose of this pull request
Fix local mode can not finish cause by #9532 .
Does this PR introduce any user-facing change?
no
How was this patch tested?
add new test
Check list
New License Guide