korlov42 commented on code in PR #6889:
URL: https://github.com/apache/ignite-3/pull/6889#discussion_r2503595427
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/fsm/Program.java:
##########
@@ -101,31 +101,55 @@ CompletableFuture<ResultT> run(Query query) {
ex = ExceptionUtils.unwrapCause(ex);
// handles exception from asynchronous
part of phase evaluation
- try {
- if (errorHandler.test(query, ex)) {
- query.executor.execute(() ->
run(query));
- }
- } catch (AssertionError | Exception ex0) {
- LOG.warn("Exception in error handler
[queryId={}]", ex0, query.id);
-
- query.onError(ex);
+ if (shouldRetry(query, ex)) {
+ query.executor.execute(() ->
run(query, state));
+ } else {
+ query.setError(ex);
+ finalizeActiveProgram(query, state);
}
return;
}
query.executor.execute(() -> {
- if (advanceQuery(query)) {
- run(query);
+ if (advanceQuery(query, state)) {
+ run(query, state);
}
});
});
break;
}
}
- } while (advanceQuery(query));
+ } while (advanceQuery(query, state));
+ }
+
+ private boolean shouldRetry(Query query, Throwable th) {
+ try {
+ if (errorHandler.test(query, th)) {
+ return true;
+ }
+ } catch (AssertionError | Exception ex) {
+ LOG.warn("Exception in error handler [queryId={}]", ex, query.id);
+
+ query.terminateExceptionally(th);
+ }
- return Commons.cast(query.resultHolder);
+ return false;
+ }
+
+ private static void finalizeActiveProgram(Query query,
ProgramExecutionState<?> executionState) {
+ ProgramExecutionHandle activeHandle =
query.activeProgram.getAndSet(null);
Review Comment:
TL;DR It's more correct to finalize state passed explicitly rather that from
Query object.
By design, program execution state was moved from Query object to dedicated
one, and passed along explicitly. `ExecutionHandle` on the other hand is a mean
to 1) propagate error which is occurred outside of the query execution yet
relates to a query, and 2) to provide a way to wait for program completion. The
fact that `ExecutionHandle` exposes original `programFinished` and not guarding
copy over `completionFuture()` is rather strictly speaking bad implementation
choice, but this is the safe environment (al least right now), and I'm taking
the risk (we may revise it at any time later). Finally, program should finalize
its own state and only, and having state object passing explicitly ensures
this. Although we verify that given state and the one in Query is actually the
same object (see the very last line of the method).
--
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]
