You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "korlov42 (via GitHub)" <gi...@apache.org> on 2025/11/03 14:11:44 UTC
[PR] IGNITE-24006 Sql. AssertionError when running ddlInsideExplicitTransactionFails test [ignite-3]
korlov42 opened a new pull request, #6889:
URL: https://github.com/apache/ignite-3/pull/6889
https://issues.apache.org/jira/browse/IGNITE-24006
-------
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
### The Review Checklist
- [ ] **Formal criteria:** TC status, codestyle, mandatory documentation. Also make sure to complete the following:
\- There is a single JIRA ticket related to the pull request.
\- The web-link to the pull request is attached to the JIRA ticket.
\- The JIRA ticket has the Patch Available state.
\- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
\- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
- [ ] **Design:** new code conforms with the design principles of the components it is added to.
- [ ] **Patch quality:** patch cannot be split into smaller pieces, its size must be reasonable.
- [ ] **Code quality:** code is clean and readable, necessary developer documentation is added if needed.
- [ ] **Tests code quality:** test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.
### Notes
- [Apache Ignite Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)
--
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-24006 Sql. AssertionError when running ddlInsideExplicitTransactionFails test [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #6889:
URL: https://github.com/apache/ignite-3/pull/6889#discussion_r2501960045
##########
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:
Why we decided to use "executionState" here?
I mean we can use `activeHandle.completionFuture()` instead of `executionState.programFinished`
--
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-24006 Sql. AssertionError when running ddlInsideExplicitTransactionFails test [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #6889:
URL: https://github.com/apache/ignite-3/pull/6889#discussion_r2503531692
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/fsm/Query.java:
##########
@@ -116,6 +123,64 @@ class Query {
this.parsedResult = parsedResult;
}
+ <ResultT> CompletableFuture<ResultT> runProgram(Program<ResultT> program) {
+ ProgramExecutionState<ResultT> state = program.createState();
+
+ if (!activeProgram.compareAndSet(null, state)) {
+ throw new SqlException(Common.INTERNAL_ERR);
Review Comment:
good catch! Forgot to add this from begging, then just switched to another place... Thanks!
--
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-24006 Sql. AssertionError when running ddlInsideExplicitTransactionFails test [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #6889:
URL: https://github.com/apache/ignite-3/pull/6889#discussion_r2503637425
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/fsm/Query.java:
##########
@@ -116,6 +123,64 @@ class Query {
this.parsedResult = parsedResult;
}
+ <ResultT> CompletableFuture<ResultT> runProgram(Program<ResultT> program) {
+ ProgramExecutionState<ResultT> state = program.createState();
+
+ if (!activeProgram.compareAndSet(null, state)) {
+ throw new SqlException(Common.INTERNAL_ERR);
+ }
+
+ program.run(this, state);
+
+ return state.resultHolder;
+ }
+
+ /**
+ * Initiates a graceful termination of the current query.
+ *
+ * <p>If the query is idle, moves it to {@link ExecutionPhase#TERMINATED} phase immediately. Otherwise, the method waits for the
+ * currently active program to complete before proceeding. Note that termination may not take effect immediately upon return. To wait
+ * for the actual completion of termination, use {@link #terminationFuture}.
+ */
+ void terminate() {
+ tryTerminate(1);
+ }
+
+ private void tryTerminate(int attemptNo) {
+ if (attemptNo >= MAX_ATTEMPTS_COUNT) {
Review Comment:
I don't think so. Initially, I got query stuck in an infinite loop because first implementation was trying to retry after `resultHolder` was completed. I missed the fact that we complete result earlier as soon as excepting raised. So, I first added MAX_ATTEMPT_COUNT, and then introduced separate `programFinished` to notify waiters that program is completed for sure.
Technically, it's still possible to get into this if there is an infinite loop running different program over the same query, and you are unlucky enough so your execution attempt lost every single race. But production code runs max ~3 program over the same query object
--
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-24006 Sql. AssertionError when running ddlInsideExplicitTransactionFails test [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 merged PR #6889:
URL: https://github.com/apache/ignite-3/pull/6889
--
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-24006 Sql. AssertionError when running ddlInsideExplicitTransactionFails test [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-24006 Sql. AssertionError when running ddlInsideExplicitTransactionFails test [ignite-3]
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #6889:
URL: https://github.com/apache/ignite-3/pull/6889#discussion_r2498043268
##########
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) {
Review Comment:
```suggestion
} catch (Throwable ex) {
```
--
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-24006 Sql. AssertionError when running ddlInsideExplicitTransactionFails test [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #6889:
URL: https://github.com/apache/ignite-3/pull/6889#discussion_r2501969093
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/fsm/Query.java:
##########
@@ -116,6 +123,64 @@ class Query {
this.parsedResult = parsedResult;
}
+ <ResultT> CompletableFuture<ResultT> runProgram(Program<ResultT> program) {
+ ProgramExecutionState<ResultT> state = program.createState();
+
+ if (!activeProgram.compareAndSet(null, state)) {
+ throw new SqlException(Common.INTERNAL_ERR);
+ }
+
+ program.run(this, state);
+
+ return state.resultHolder;
+ }
+
+ /**
+ * Initiates a graceful termination of the current query.
+ *
+ * <p>If the query is idle, moves it to {@link ExecutionPhase#TERMINATED} phase immediately. Otherwise, the method waits for the
+ * currently active program to complete before proceeding. Note that termination may not take effect immediately upon return. To wait
+ * for the actual completion of termination, use {@link #terminationFuture}.
+ */
+ void terminate() {
+ tryTerminate(1);
+ }
+
+ private void tryTerminate(int attemptNo) {
+ if (attemptNo >= MAX_ATTEMPTS_COUNT) {
Review Comment:
Just curious - can I somehow simulate repeated attempts (attempts > 2)?
I mean running specific query and/or adding sleeps somewhere?)
--
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-24006 Sql. AssertionError when running ddlInsideExplicitTransactionFails test [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #6889:
URL: https://github.com/apache/ignite-3/pull/6889#discussion_r2501901341
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/fsm/Query.java:
##########
@@ -116,6 +123,64 @@ class Query {
this.parsedResult = parsedResult;
}
+ <ResultT> CompletableFuture<ResultT> runProgram(Program<ResultT> program) {
+ ProgramExecutionState<ResultT> state = program.createState();
+
+ if (!activeProgram.compareAndSet(null, state)) {
+ throw new SqlException(Common.INTERNAL_ERR);
Review Comment:
mb add some message or comment to clarify what it means?
##########
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:
Why we need "executionState" here?
I mean we can use `activeHandle.completionFuture()` instead of `executionState.programFinished`
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/fsm/Query.java:
##########
@@ -116,6 +123,64 @@ class Query {
this.parsedResult = parsedResult;
}
+ <ResultT> CompletableFuture<ResultT> runProgram(Program<ResultT> program) {
+ ProgramExecutionState<ResultT> state = program.createState();
+
+ if (!activeProgram.compareAndSet(null, state)) {
+ throw new SqlException(Common.INTERNAL_ERR);
+ }
+
+ program.run(this, state);
+
+ return state.resultHolder;
+ }
+
+ /**
+ * Initiates a graceful termination of the current query.
+ *
+ * <p>If the query is idle, moves it to {@link ExecutionPhase#TERMINATED} phase immediately. Otherwise, the method waits for the
+ * currently active program to complete before proceeding. Note that termination may not take effect immediately upon return. To wait
+ * for the actual completion of termination, use {@link #terminationFuture}.
+ */
+ void terminate() {
+ tryTerminate(1);
+ }
+
+ private void tryTerminate(int attemptNo) {
+ if (attemptNo >= MAX_ATTEMPTS_COUNT) {
Review Comment:
Just curious - can I somehow simulate repeated attempts (attempts > 2)?
--
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-24006 Sql. AssertionError when running ddlInsideExplicitTransactionFails test [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #6889:
URL: https://github.com/apache/ignite-3/pull/6889#discussion_r2501901341
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/fsm/Query.java:
##########
@@ -116,6 +123,64 @@ class Query {
this.parsedResult = parsedResult;
}
+ <ResultT> CompletableFuture<ResultT> runProgram(Program<ResultT> program) {
+ ProgramExecutionState<ResultT> state = program.createState();
+
+ if (!activeProgram.compareAndSet(null, state)) {
+ throw new SqlException(Common.INTERNAL_ERR);
Review Comment:
mb add some message or comment to clarify what's happening?
--
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