You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "valepakh (via GitHub)" <gi...@apache.org> on 2026/04/02 16:15:09 UTC
[PR] IGNITE-27855 Add JobExecutionContext#cancellationToken [ignite-3]
valepakh opened a new pull request, #7922:
URL: https://github.com/apache/ignite-3/pull/7922
https://issues.apache.org/jira/browse/IGNITE-27855
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27855 Add JobExecutionContext#cancellationToken [ignite-3]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27855 Add JobExecutionContext#cancellationToken [ignite-3]
Posted by "Pochatkin (via GitHub)" <gi...@apache.org>.
Pochatkin merged PR #7922:
URL: https://github.com/apache/ignite-3/pull/7922
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27855 Add JobExecutionContext#cancellationToken [ignite-3]
Posted by "valepakh (via GitHub)" <gi...@apache.org>.
valepakh commented on code in PR #7922:
URL: https://github.com/apache/ignite-3/pull/7922#discussion_r3032317394
##########
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:
This is fine — the message comes from our own SQL engine and is stable. Exact matching ensures we catch unintended message changes.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org