Copilot commented on code in PR #7922:
URL: https://github.com/apache/ignite-3/pull/7922#discussion_r3029521150
##########
modules/compute/src/test/java/org/apache/ignite/internal/compute/executor/ComputeExecutorTest.java:
##########
@@ -449,6 +450,38 @@ public CompletableFuture<Void>
executeAsync(JobExecutionContext context, Object.
}
}
+ @Test
+ void cancelAsyncJobViaCancellationToken() {
+ JobExecutionInternal<?> execution =
executeJob(CancellationTokenJob.class);
+
+ JobState executingState = await().until(execution::state,
jobStateWithStatus(EXECUTING));
+
+ // The job registers a cancel action on the cancellation token that
completes the future.
+ // When cancel() is called, the token fires the action synchronously,
completing the job with a result.
+ execution.cancel();
+
+ await().until(
+ execution::state,
+ jobStateWithStatusAndCreateTimeStartTime(COMPLETED,
executingState.createTime(), executingState.startTime())
+ );
Review Comment:
This test asserts only that the state becomes `COMPLETED`, but it doesn’t
validate that the cancellation-token action actually ran (i.e., the future
completed with the expected value `42`). Adding an assertion on the job result
(or equivalent observable output) would make the test enforce the intended
behavior rather than just a terminal state.
```suggestion
);
assertThat(execution.resultAsync(), willBe(42));
```
##########
modules/compute/src/main/java/org/apache/ignite/internal/compute/executor/JobExecutionInternal.java:
##########
@@ -80,10 +80,13 @@ public JobState state() {
/**
* Cancel job execution.
*
+ * <p>First, cancels operations registered on the job's cancellation token
(e.g., SQL queries).
+ * Then, interrupts the worker thread as a fallback for jobs that don't
use the token.
+ *
* @return {@code true} if job was successfully cancelled.
*/
public boolean cancel() {
- isInterrupted.set(true);
+ cancelHandle.cancelAsync();
Review Comment:
The javadoc guarantees ordering ('first' cancel token operations, 'then'
interrupt/cancel the worker), but `cancelAsync()` is not awaited and the
returned future is ignored, so `execution.cancel()` may run before token
handlers complete. Consider using a synchronous cancel (if available) or
waiting for `cancelAsync()` completion before calling `execution.cancel()`,
and/or adjust the javadoc to match the actual ordering.
```suggestion
cancelHandle.cancelAsync().join();
```
##########
modules/compute/src/integrationTest/java/org/apache/ignite/internal/compute/ItComputeBaseTest.java:
##########
@@ -1019,4 +1080,10 @@ private static Matcher<Exception>
computeJobCancelledException() {
.or(instanceOf(CancellationException.class))
);
}
+
+ private static Matcher<Exception> sqlCancelledException() {
+ return traceableException(SqlException.class)
+ .withCode(is(EXECUTION_CANCELLED_ERR))
+ .withMessage(is("The query was cancelled while executing."));
Review Comment:
Matching the exception message exactly makes the test brittle (wording
changes, localization, or upstream SQL changes can break it without changing
semantics). Prefer asserting on the error code (already done) and, if needed,
using a weaker message assertion (e.g., `containsString`) or dropping the
message check.
```suggestion
.withMessage(containsString("cancelled while executing"));
```
--
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]
