IGNITE-23499 Integrate cancellation of compute job with sql kill handler. #4898

xtern · 2024-12-13T13:29:11Z

https://issues.apache.org/jira/browse/IGNITE-23499

…ler.

zstan · 2024-12-17T06:02:32Z

...rc/integrationTest/java/org/apache/ignite/internal/sql/engine/kill/ItSqlKillCommandTest.java

@@ -157,4 +283,21 @@ private static AsyncSqlCursor<InternalSqlRow> executeQueryInternal(Ignite node,

        return await(fut);
    }
+
+    private static class InfiniteJob implements ComputeJob<Void, Void> {


can we avoid copy-paste ? can it be public ItComputeSystemViewTest.InfiniteJob ?

Done, thanks

zstan · 2024-12-17T06:23:35Z

...rc/integrationTest/java/org/apache/ignite/internal/sql/engine/kill/ItSqlKillCommandTest.java

+        JobDescriptor<Void, Void> job = JobDescriptor.builder(InfiniteJob.class).units(List.of()).build();
+        JobExecution<Void> execution = node.compute().submit(JobTarget.node(clusterNode(node)), job, null);
+
+        Awaitility.await().until(execution::stateAsync, willBe(jobStateWithStatus(EXECUTING)));


why do we need to wait status here ? let`s change this test - kill job immediately.

Thanks, removed this condition.

why do we need to wait status here ?

I thought that we need to wait until task will be deployed and I assumed that if this is not done, there possible flaky failures (that we will cancel a non-existent task). But locally I don't see flaky failures so I removed this check here and in the killComputeJobFromRemote test.

zstan · 2024-12-17T06:31:37Z

...rc/integrationTest/java/org/apache/ignite/internal/sql/engine/kill/ItSqlKillCommandTest.java

+        Awaitility.await().until(execution::stateAsync, willBe(jobStateWithStatus(CANCELED)));
+
+        assertThat(executeKillJob(node, jobId), is(false));
+        assertThat(executeKill(node, COMPUTE, jobId, true), is(true));
    }

    @Test
    public void killQueryFromRemote() {


seems we have no "real" tests for killing long-living queries ? WDYT, do we need such a tests ? Because we already have real infinite job in compute, but it not tested in sql.

Do you mean something that doesn't fit on a single page?
i.e. SELECT x FROM system_range(0, 100000)?

zstan · 2024-12-17T06:35:55Z

...rc/integrationTest/java/org/apache/ignite/internal/sql/engine/kill/ItSqlKillCommandTest.java

+    private static boolean executeKillJob(Ignite node, UUID jonId) {
+        return executeKill(node, COMPUTE, jonId, false);


Suggested change

private static boolean executeKillJob(Ignite node, UUID jonId) {

return executeKill(node, COMPUTE, jonId, false);

private static boolean executeKillJob(Ignite node, UUID jobId) {

return executeKill(node, COMPUTE, jobId, false);

Fixed, thanks

zstan · 2024-12-17T07:20:50Z

...engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/kill/KillCommandHandler.java

@@ -102,7 +102,7 @@ public CompletableFuture<Boolean> handle(KillCommand cmd) {

        CompletableFuture<Boolean> killFut = invokeCancel(handler, cmd.operationId());

-        if (killFut.isDone() || !cmd.noWait()) {
+        if (killFut.isCompletedExceptionally() || !cmd.noWait()) {


I miss smth here, if noWait == true - we must return immediatelly i.e. return CompletableFuture.completedFuture(true); otherwise need to return killFut, isn`t it ?

Yes

if noWait == true - we must return immediatelly i.e. return CompletableFuture.completedFuture(true);

...except handling exception described in javadoc

@throws IllegalArgumentException If the operation identifier is not in the correct format.

For example ``KILL QUERY '123' NO WAIT` throws exception instead of returning true 🤔

zstan · 2024-12-17T08:48:02Z

Also seems we have uncompleted and probably erroneous (sql mention) java doc here: OperationKillHandler

    /**
     * Returns whether the handler can abort operations only on local node or across the entire cluster.
     *
     * <p>If a handler can only abort operations on the local node, then distributed coordination will
     * be performed by the SQL engine. <-???
     *
     * @return {@code True} if the handler can abort operations only on local node,
     *         {@code false} if the handler can abort operations across the entire cluster (in such a case . <- ???
     */
    boolean local();

xtern · 2024-12-17T09:14:59Z

Also seems we have uncompleted and probably erroneous (sql mention) java doc here: OperationKillHandler

    /**
     * Returns whether the handler can abort operations only on local node or across the entire cluster.
     *
     * <p>If a handler can only abort operations on the local node, then distributed coordination will
     * be performed by the SQL engine. <-???
     *
     * @return {@code True} if the handler can abort operations only on local node,
     *         {@code false} if the handler can abort operations across the entire cluster (in such a case . <- ???
     */
    boolean local();

This is done intentionally. If a local handler is registered, our KillCommandHandler (part of sql-engine) will call local handlers across the cluster.
Check this comment #4799 (comment)

IGNITE-23499 Integrate cancellation of compute job with sql kill hand…

966d2f9

…ler.

xtern force-pushed the ignite-23499 branch from d62c624 to 966d2f9 Compare December 13, 2024 13:55

IGNITE-23499 (minor) Use implementation.

3288629

ygerzhedovich approved these changes Dec 16, 2024

View reviewed changes

zstan reviewed Dec 17, 2024

View reviewed changes

IGNITE-23499 Review notes.

c8291b1

IGNITE-23499 Review notes.

3b726e2

xtern added 2 commits December 17, 2024 12:44

IGNITE-23499 Review note - fix javadoc.

8c62b3c

IGNITE-23459 Fix code style.

8dbb7b8

zstan approved these changes Dec 17, 2024

View reviewed changes

xtern added 3 commits December 17, 2024 14:17

Merge remote-tracking branch 'origin/main' into ignite-23499

535e561

IGNITE-23459 (minor) Code comment.

bdcbea7

IGNITE-23499 (minor) Improved exception check test a bit.

6606787

xtern merged commit 977f6c4 into apache:main Dec 17, 2024
1 check passed

xtern deleted the ignite-23499 branch December 17, 2024 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IGNITE-23499 Integrate cancellation of compute job with sql kill handler. #4898

IGNITE-23499 Integrate cancellation of compute job with sql kill handler. #4898

xtern commented Dec 13, 2024

zstan Dec 17, 2024

xtern Dec 17, 2024

zstan Dec 17, 2024

xtern Dec 17, 2024 •

edited

Loading

zstan Dec 17, 2024

xtern Dec 17, 2024

zstan Dec 17, 2024

xtern Dec 17, 2024

zstan Dec 17, 2024

xtern Dec 17, 2024

zstan commented Dec 17, 2024

xtern commented Dec 17, 2024 •

edited

Loading

		private static boolean executeKillJob(Ignite node, UUID jonId) {
		return executeKill(node, COMPUTE, jonId, false);

IGNITE-23499 Integrate cancellation of compute job with sql kill handler. #4898

IGNITE-23499 Integrate cancellation of compute job with sql kill handler. #4898

Conversation

xtern commented Dec 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xtern Dec 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zstan commented Dec 17, 2024

xtern commented Dec 17, 2024 • edited Loading

xtern Dec 17, 2024 •

edited

Loading

xtern commented Dec 17, 2024 •

edited

Loading