lowka commented on code in PR #3953:
URL: https://github.com/apache/ignite-3/pull/3953#discussion_r1650982047


##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/QueryCancel.java:
##########
@@ -32,21 +36,66 @@ public class QueryCancel {
 
     private boolean canceled;
 
+    private boolean timeout;
+
     /**
-     * Adds a cancel action.
+     * Adds a cancel action. If operation has already been canceled, throws a 
{@link QueryCancelledException}.
+     *
+     * <p>NOTE: If the operation is cancelled, this method will immediately 
invoke the given action
+     * and then throw a {@link QueryCancelledException}.
      *
-     * @param clo Add cancel action.
+     * @param clo Cancel action.
      */
     public synchronized void add(Cancellable clo) throws 
QueryCancelledException {
         assert clo != null;
 
         if (canceled) {
-            throw new QueryCancelledException();
+            // Immediately invoke a cancel action, if already cancelled. 
+            // Otherwise the caller is required to catch 
QueryCancelledException and call an action manually.
+            try {
+                clo.cancel(timeout);
+            } catch (Exception ignore) {
+                // Do nothing
+            }
+
+            String message = timeout ? QueryCancelledException.TIMEOUT_MSG : 
QueryCancelledException.CANCEL_MSG;
+            throw new QueryCancelledException(message);
         }
 
         cancelActions.add(clo);
     }
 
+    /**
+     * Removes the given callback.
+     *
+     * @param clo Callback.
+     */
+    public synchronized void remove(Cancellable clo) {
+        assert clo != null;
+
+        cancelActions.remove(clo);
+    }
+
+    /**
+     * Schedules a timeout action (a call to {@link #timeout()}) after {@code 
timeoutMillis} milliseconds.
+     *
+     * @param scheduler Scheduler to trigger an action.
+     * @param timeoutMillis Timeout in milliseconds.
+     * @return Future that will be completed when the timeout is reached.
+     */
+    public CompletableFuture<Void> addTimeout(ScheduledExecutorService 
scheduler, long timeoutMillis) {
+        CompletableFuture<Void> fut = new CompletableFuture<>();
+        fut.thenAccept((r) -> timeout());
+
+        ScheduledFuture<?> f = scheduler.schedule(() -> {
+            fut.complete(null);
+        }, timeoutMillis, MILLISECONDS);
+
+        add((timeout) -> f.cancel(false));

Review Comment:
   Just to make this clear. For every statement we create a QueryCancel thing 
that stores actions to be called when a statement is cancelled.  
   
   A statement is cancelled on initiator in case of an error. 
DistributedQueryManage calls query cancel : cancel method. QueryCancel cancel 
calls all cancellation callbacks.
   
   This commit introduced a timeout method on QueryCancel to distinguish 
between a cancellation due to some exception and a time out.
   
   - QueryProcessor creates an instance of QueryCancel q.
   - it schedules a timeout action for a query that calls q::timeout (to 
indicate that timeout occurred and a statement should be cancelled)
   - when a statement is cancelled due to some exception by its initiator a 
scheduled future should be cancelled as well.
   - when a statement is cancelled due to timeout we call q::timeout. That 
calls cancel of a future that has no effect.
   
   
   



-- 
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]

Reply via email to