timoninmaxim commented on code in PR #11528:
URL: https://github.com/apache/ignite/pull/11528#discussion_r1793191469
##########
modules/core/src/main/java/org/apache/ignite/internal/jdbc/thin/JdbcThinConnection.java:
##########
@@ -518,7 +607,10 @@ private void checkCursorOptions(int resSetType, int
resSetConcurrency) throws SQ
if (autoCommit)
Review Comment:
Should we enable this behavior? I suppose `commit()` should perform commit
in the auto commit mode.
##########
modules/core/src/main/java/org/apache/ignite/internal/jdbc/thin/JdbcThinConnection.java:
##########
@@ -162,6 +169,9 @@ public class JdbcThinConnection implements Connection {
/** Zero timeout as query timeout means no timeout. */
static final int NO_TIMEOUT = 0;
+ /** No transaction id. */
+ public static final int NO_TX = 0;
Review Comment:
NO -> NONE. Similar to Connection#TRANSACTION_NONE
##########
modules/core/src/main/java/org/apache/ignite/internal/jdbc/thin/JdbcThinConnection.java:
##########
@@ -338,6 +351,72 @@ boolean isStream() {
return streamState != null;
}
+ /** @return {@code True} if there are open transaction, {@code false}
otherwise. */
Review Comment:
there is
##########
modules/core/src/main/java/org/apache/ignite/internal/jdbc/thin/JdbcThinConnection.java:
##########
@@ -338,6 +351,72 @@ boolean isStream() {
return streamState != null;
}
+ /** @return {@code True} if there are open transaction, {@code false}
otherwise. */
+ public boolean isTxOpen() {
Review Comment:
Usages can be inlined. Method is required only for tests. This should be
avoided.
##########
modules/core/src/main/java/org/apache/ignite/internal/jdbc/thin/JdbcThinConnection.java:
##########
@@ -338,6 +351,72 @@ boolean isStream() {
return streamState != null;
}
+ /** @return {@code True} if there are open transaction, {@code false}
otherwise. */
+ public boolean isTxOpen() {
+ return txCtx != null;
+ }
+
+ /** @return {@code True} if transactions supported by the server, {@code
false} otherwise. */
Review Comment:
by -> on. Should match method name. Please fix in other docs also
##########
modules/core/src/main/java/org/apache/ignite/internal/jdbc/thin/JdbcThinConnection.java:
##########
@@ -228,8 +238,8 @@ public class JdbcThinConnection implements Connection {
/** Mutex. */
private final Object mux = new Object();
- /** Ignite endpoint to use within transactional context. */
- private volatile JdbcThinTcpIo txIo;
+ /** Transactional context. */
+ private volatile TxContext txCtx;
Review Comment:
Looks like it can be non-volatile.
##########
modules/core/src/main/java/org/apache/ignite/internal/jdbc/thin/JdbcThinConnection.java:
##########
@@ -338,6 +351,72 @@ boolean isStream() {
return streamState != null;
}
+ /** @return {@code True} if there are open transaction, {@code false}
otherwise. */
+ public boolean isTxOpen() {
+ return txCtx != null;
+ }
+
+ /** @return {@code True} if transactions supported by the server, {@code
false} otherwise. */
+ boolean txSupportedOnServer() {
+ return (singleIo != null || !ios.isEmpty()) &&
defaultIo().isTxAwareQueriesSupported();
+ }
+
+ /** @return {@code True} if certain isolation level supported by the
server, {@code false} otherwise. */
+ boolean isolationLevelSupported(int level) throws SQLException {
+ if (level == TRANSACTION_NONE)
+ return true;
+
+ if (level == TRANSACTION_READ_UNCOMMITTED)
Review Comment:
`isIsolationLevelSupported` should return `false`. I think this if is useless
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/odbc/ClientTxSupport.java:
##########
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.odbc;
+
+import org.apache.ignite.IgniteCheckedException;
+import org.apache.ignite.internal.IgniteInternalFuture;
+import
org.apache.ignite.internal.processors.cache.distributed.near.GridNearTxLocal;
+import
org.apache.ignite.internal.processors.cache.transactions.IgniteInternalTx;
+import
org.apache.ignite.internal.processors.platform.client.tx.ClientTxContext;
+import org.apache.ignite.internal.util.future.GridFinishedFuture;
+import org.apache.ignite.transactions.TransactionConcurrency;
+import org.apache.ignite.transactions.TransactionIsolation;
+
+/** */
+public interface ClientTxSupport {
+ /** @return Client connection context. */
+ ClientListenerAbstractConnectionContext context();
Review Comment:
Let's add `context` as param in methods `startClientRequest` and `endTxAsync`
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/odbc/ClientListenerAbstractConnectionContext.java:
##########
@@ -123,6 +131,8 @@ protected void authenticate(GridNioSession ses, String
user, String pwd) throws
/** {@inheritDoc} */
@Override public void onDisconnected() {
+ cleanupTxs();
Review Comment:
It's cleaned twice, here and in `ClientConnectionContext#onDisconnected`
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/odbc/ClientListenerAbstractConnectionContext.java:
##########
@@ -149,6 +159,39 @@ public String clientDescriptor() {
return clientDesc;
}
+ /**
+ * Next transaction id for this connection.
+ */
+ public int nextTxId() {
+ int txId = txIdSeq.incrementAndGet();
+
+ return txId == NO_TX ? txIdSeq.incrementAndGet() : txId;
+ }
+
+ /**
+ * Transaction context by transaction id.
+ *
+ * @param txId Tx ID.
+ */
+ public abstract ClientTxContext txContext(int txId);
Review Comment:
`@Nullable`
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/odbc/ClientListenerAbstractConnectionContext.java:
##########
@@ -149,6 +159,39 @@ public String clientDescriptor() {
return clientDesc;
}
+ /**
+ * Next transaction id for this connection.
+ */
+ public int nextTxId() {
+ int txId = txIdSeq.incrementAndGet();
+
+ return txId == NO_TX ? txIdSeq.incrementAndGet() : txId;
Review Comment:
`txId == NO_TX` -> `txId <= 0`?
##########
modules/core/src/main/java/org/apache/ignite/internal/jdbc/thin/JdbcThinConnection.java:
##########
@@ -338,6 +351,72 @@ boolean isStream() {
return streamState != null;
}
+ /** @return {@code True} if there are open transaction, {@code false}
otherwise. */
+ public boolean isTxOpen() {
+ return txCtx != null;
+ }
+
+ /** @return {@code True} if transactions supported by the server, {@code
false} otherwise. */
+ boolean txSupportedOnServer() {
Review Comment:
This method is used match, looks like we can cache it while establishing the
connection.
##########
modules/core/src/main/java/org/apache/ignite/internal/jdbc/thin/JdbcThinConnection.java:
##########
@@ -2464,4 +2608,82 @@ boolean handleResult(long reqId, JdbcResult res) {
return handled;
}
}
+
+ /** Transaction context. */
+ public class TxContext {
Review Comment:
Public nested class is bad practice. It looks like that `JdbcThinStatement`
needs only current `txId` instead of full context.
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/odbc/jdbc/JdbcThinFeature.java:
##########
@@ -33,7 +33,10 @@ public enum JdbcThinFeature implements ThinProtocolFeature {
CUSTOM_OBJECT(2),
/** Add ability to set explicit query timeout on the cluster node by the
JDBC client. */
- QUERY_TIMEOUT(3);
+ QUERY_TIMEOUT(3),
+
+ /** Transaction aware queries. */
+ TX_AWARE_QUERIES(4);
Review Comment:
Just `TRANSACTIONS`?
##########
modules/core/src/main/java/org/apache/ignite/internal/jdbc/thin/JdbcThinConnection.java:
##########
@@ -898,6 +1008,33 @@ private void checkCursorOptions(int resSetType, int
resSetConcurrency) throws SQ
return netTimeout;
}
+ /** Store transaction context. */
Review Comment:
Store statement to transaction context?
##########
modules/core/src/main/java/org/apache/ignite/internal/jdbc/thin/JdbcThinConnection.java:
##########
@@ -2464,4 +2608,82 @@ boolean handleResult(long reqId, JdbcResult res) {
return handled;
}
}
+
+ /** Transaction context. */
+ public class TxContext {
+ /** IO to transaction coordinator. */
+ final JdbcThinTcpIo txIo;
+
+ /** Transaction id. */
+ final int txId;
Review Comment:
txSeqId? This is not an actual transaction id (IgniteTxAdapter#xid), name
can be misleading.
##########
modules/core/src/main/java/org/apache/ignite/internal/jdbc/thin/ConnectionProperties.java:
##########
@@ -560,4 +561,40 @@ public void
setPartitionAwarenessPartitionDistributionsCacheSize(
* @param qryEngine SQL Query engine name.
*/
public void setQueryEngine(String qryEngine);
+
+ /**
+ * @return Transaction concurrency value.
+ */
+ public TransactionConcurrency getTransactionConcurrency();
+
+ /**
+ * Sets transaction concurrency.
+ *
+ * @param transactionConcurrency Transaction concurrecny.
+ */
+ public void setTransactionConcurrency(String transactionConcurrency);
+
+ /**
+ * @return Transaction timeout in milliseconds.
+ */
+ public int getTransactionTimeout();
+
+ /**
+ * Sets transaction timeout in milliseconds.
+ *
+ * @param transactionTimeout Transaction timeout in millicesonds.
+ */
+ public void setTransactionTimeout(int transactionTimeout) throws
SQLException;
Review Comment:
let's use long, similar to thin client and ignite transactions.
--
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]
