korlov42 commented on code in PR #3511:
URL: https://github.com/apache/ignite-3/pull/3511#discussion_r1597969797
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/QueryTransactionWrapperSelfTest.java:
##########
Review Comment:
let's add tests to ensure TransactionWrapper/Context interacts with
TransactionInflights as expected
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/tx/QueryTransactionContext.java:
##########
@@ -45,19 +59,26 @@ public QueryTransactionContext(IgniteTransactions
transactions, @Nullable Intern
* @return Transaction wrapper.
*/
public QueryTransactionWrapper getOrStartImplicit(SqlQueryType queryType) {
- InternalTransaction outerTx = tx;
+ InternalTransaction transaction = tx;
+ boolean implicit;
- if (outerTx == null) {
- return new QueryTransactionWrapperImpl((InternalTransaction)
transactions.begin(
- new TransactionOptions().readOnly(queryType !=
SqlQueryType.DML)), true);
+ if (transaction == null) {
+ implicit = true;
+ transaction = (InternalTransaction) transactions.begin(
+ new TransactionOptions().readOnly(queryType !=
SqlQueryType.DML));
+ } else {
+ implicit = false;
+ validateStatement(queryType, transaction.isReadOnly());
}
- validateStatement(queryType, outerTx.isReadOnly());
+ // Adding inflights only for read-only transactions.
Review Comment:
tbh, this comment doesn't bring new information. The fact that we track only
RO transactions is clear from the very next line. It's better to mention why do
we omit RW transactions here
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/tx/ScriptTransactionContext.java:
##########
@@ -96,7 +103,11 @@ public CompletableFuture<Void>
handleControlStatement(SqlNode node) {
boolean readOnly = ((IgniteSqlStartTransaction) node).getMode() ==
IgniteSqlStartTransactionMode.READ_ONLY;
InternalTransaction tx = (InternalTransaction)
queryTxCtx.transactions().begin(new TransactionOptions().readOnly(readOnly));
- this.wrapper = new ScriptTransactionWrapperImpl(tx);
+ if (readOnly && !transactionInflights.addInflight(tx.id(),
readOnly)) {
Review Comment:
let's mention why do we omit RW transactions here
--
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-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
