nzw921rx commented on code in PR #11000:
URL: https://github.com/apache/seatunnel/pull/11000#discussion_r3367515349
##########
seatunnel-engine/seatunnel-engine-client/src/test/java/org/apache/seatunnel/engine/client/SeaTunnelEngineClusterRoleTest.java:
##########
@@ -454,12 +462,14 @@ public void
testWorkerIsFirstMemberThenGetJobDetailStatus() {
.listJobStatus(true)
.contains("RUNNING")));
jobClient.cancelJob(jobId);
- await().pollDelay(10000, TimeUnit.MILLISECONDS)
- .atMost(60000, TimeUnit.MILLISECONDS)
+ await().atMost(120000, TimeUnit.MILLISECONDS)
.untilAsserted(
- () ->
- Assertions.assertEquals(
- "CANCELED",
jobClient.getJobStatus(jobId)));
+ () -> {
+ String status = jobClient.getJobStatus(jobId);
+ Assertions.assertTrue(
+ "CANCELED".equals(status) ||
"FAILED".equals(status),
+ "Expected terminal state but was: " +
status);
+ });
Review Comment:
> `FAILED` is accepted here because `cancelJob()` only asks the cluster to
stop the job; it does not guarantee that the job can no longer reach another
terminal state first. In this test path, a task can already be failing while
the cancel request is in flight, so the stable contract to assert is that the
job leaves `RUNNING` / `CANCELLING` and reaches a terminal state. Accepting
both `CANCELED` and `FAILED` keeps the test aligned with that real race instead
of baking in a stricter state transition than the runtime guarantees.
1. This will result in ambiguity in the cancelJob() state, which does not
conform to the normal flow of job states
2. During the continuous evolution of the project, there may be job failures
caused by true cancelJob() failures. This use case will result in false green
--
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]
