IGNITE-21868 Moved the sql RO inflights handling from SqlQueryProcess… #3511
Conversation
…or to QueryTransactionContext and QueryTransactionWrapper
denis-chudov
force-pushed
the
ignite-21868
branch
from
0fb4661 to
08e0805
Compare
...l-engine/src/main/java/org/apache/ignite/internal/sql/engine/tx/QueryTransactionContext.java
Outdated
Show resolved
Hide resolved
...-engine/src/main/java/org/apache/ignite/internal/sql/engine/tx/ScriptTransactionContext.java
Outdated
Show resolved
Hide resolved
...ine/src/test/java/org/apache/ignite/internal/sql/engine/QueryTransactionWrapperSelfTest.java
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public CompletableFuture<Void> commitImplicit() { | ||
| if (transaction.isReadOnly() && committedImplicit.compareAndSet(false, true)) { |
There was a problem hiding this comment.
do we really need this CaS? Wouldn't it work correctly without committedImplicit?
There was a problem hiding this comment.
getOrStartImplicit and commitImplicit are called inside of SQL code every time when cursor is opened/closed respectively.
getOrStartImplicit may use an existing txn or start a new one.
commitImplicit is always used only to commit the explicit transaction on cursor close. It is no-op for explicit txns, but anyway it's called every time when cursor is closed. Unfortunately commitImplicit is idempotent and SQL code relies on this fact, removeInflight is not - it throws assertion if there are no more inflights. So we would have assertion errors in SQL code without this CAS. Also, commitImplicit is not no-op for explicit txns now: it should remove inflights on cursor close for both explicit and implicit RO txns.
| throw new IllegalStateException("Unknown transaction target state: " + txState); | ||
| } | ||
|
|
||
| if (managedTx.isReadOnly() && completedTx.compareAndSet(false, true)) { |
There was a problem hiding this comment.
IIRC removeInflight is idempotent, no need to have a CAS to protect it
There was a problem hiding this comment.
It's not, we have an assertion that there are any inflights to remove. Also check the comment above
| transaction = txManager.begin(observableTimeTracker, readOnly); | ||
| result = new QueryTransactionWrapperImpl(transaction, true, transactionInflights); | ||
| } else { | ||
| transaction = tx.unwrap(); |
There was a problem hiding this comment.
Sorry, I didn't find this branch covered with tests.
There was a problem hiding this comment.
It is the part of QueryTransactionWrapperSelfTest#testQueryTransactionWrapperTxInflightsInteraction regarding explicitRwTxCtx and explicitRoTxCtx. Actually, the only purpose of both these branches is to assign transaction and result to not duplicate the code below adding the inflight.
3 participants
https://issues.apache.org/jira/browse/IGNITE-21868
Moved the adding/removing the transaction inflights for RO transactions from SqlQueryProcessor to QueryTransactionContext.