Copilot commented on code in PR #9549:
URL: https://github.com/apache/seatunnel/pull/9549#discussion_r2194033991
##########
seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/CoordinatorServiceTest.java:
##########
@@ -199,12 +199,30 @@ public void testClearCoordinatorService() {
throw new RuntimeException(e);
}
+ int scheduleRunnerThreadCount =
+ (int)
+ Thread.getAllStackTraces().keySet().stream()
+ .filter(
+ thread ->
+ thread.getName()
+
.startsWith("pending-job-schedule-runner"))
+ .count();
+ Assertions.assertTrue(scheduleRunnerThreadCount > 0);
+
coordinatorService.clearCoordinatorService();
- // because runningJobMasterMap is empty and we have no
JobHistoryServer, so return
+ // because runningJobMasterMap is empty, and we have no
JobHistoryServer, so return
// UNKNOWABLE.
-
Assertions.assertTrue(JobStatus.UNKNOWABLE.equals(coordinatorService.getJobStatus(jobId)));
+ Assertions.assertEquals(JobStatus.UNKNOWABLE,
coordinatorService.getJobStatus(jobId));
coordinatorServiceTest.shutdown();
+
+ Assertions.assertEquals(
+ scheduleRunnerThreadCount - 1,
+ Thread.getAllStackTraces().keySet().stream()
+ .filter(
+ thread ->
+
thread.getName().startsWith("pending-job-schedule-runner"))
+ .count());
Review 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.
```suggestion
await().atMost(10, TimeUnit.SECONDS)
.untilAsserted(() -> Assertions.assertEquals(
scheduleRunnerThreadCount - 1,
Thread.getAllStackTraces().keySet().stream()
.filter(
thread ->
thread.getName().startsWith("pending-job-schedule-runner"))
.count()));
```
##########
seatunnel-engine/seatunnel-engine-server/src/main/java/org/apache/seatunnel/engine/server/CoordinatorService.java:
##########
@@ -230,6 +230,8 @@ private void startPendingJobScheduleThread() {
while (true) {
try {
pendingJobSchedule();
+ } catch (InterruptedException interrupted) {
+ throw new RuntimeException(interrupted);
Review 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.
```suggestion
Thread.currentThread().interrupt();
break;
```
--
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]
