lowka commented on code in PR #3953:
URL: https://github.com/apache/ignite-3/pull/3953#discussion_r1650754074
##########
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:
> assert timeout == false || f.isDone(); // <-- is this correct?
These assertions are not necessary, since Future::cancel is idempotent.
`QueryCancel` expects its callbacks to be idempotent as well. It is a in
multithreaded environment after all, if multiple actors want to terminate a
query, only one of them can succeed (other actors should just accept the fact
that someone else has already done the job they tried to do).
--
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]
