sanpwc commented on code in PR #6413:
URL: https://github.com/apache/ignite-3/pull/6413#discussion_r2287257360
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/configuration/TransactionConfigurationSchema.java:
##########
@@ -39,7 +41,7 @@ public class TransactionConfigurationSchema {
@Range(min = 1)
@Value(hasDefault = true)
@PublicName(legacyNames = "readWriteTimeout")
- public final long readWriteTimeoutMillis = TimeUnit.SECONDS.toMillis(30);
+ public final long readWriteTimeoutMillis =
TimeUnit.SECONDS.toMillis(DEFAULT_RW_TX_TIMEOUT_SECONDS);
Review Comment:
What's the point of introducing `IgniteTransactionDefaults`?
TransactionConfiguration should be an only source of truth for txTimeout
(which should have default default value if not initialized), thus direct usage
of `DEFAULT_RW_TX_TIMEOUT_SECONDS` like
`long initialTimeout = options == null ?
TimeUnit.SECONDS.toMillis(DEFAULT_RW_TX_TIMEOUT_SECONDS) :
options.timeoutMillis();` should be prohibited.
##########
modules/api/src/main/java/org/apache/ignite/tx/IgniteTransactions.java:
##########
@@ -125,7 +127,9 @@ default CompletableFuture<Transaction> beginAsync() {
* }
* </pre>
*
- * <p>If the closure is executed normally (no exceptions) the transaction
is automatically committed.
+ * <p>If the closure is executed normally (no exceptions) the transaction
is automatically committed. In a case of exception, the
+ * closure will be retried automatically within the transaction timeout,
so it must be pure function. If the transaction timeout
Review Comment:
Any transaction should be pure function, meaning that no effect should be
observed in case of rollback. Are we going to retry the transaction in case of
any exception, e.g. ConstraintViolationException?
##########
modules/api/src/main/java/org/apache/ignite/tx/IgniteTransactions.java:
##########
@@ -257,26 +267,9 @@ default <T> T runInTransaction(Function<Transaction, T>
clo) throws TransactionE
* @throws TransactionException If a transaction can't be finished
successfully.
*/
default <T> T runInTransaction(Function<Transaction, T> clo, @Nullable
TransactionOptions options) throws TransactionException {
- Objects.requireNonNull(clo);
-
- Transaction tx = begin(options);
-
- try {
- T ret = clo.apply(tx);
-
- tx.commit();
-
- return ret;
- } catch (Throwable t) {
- // TODO FIXME https://issues.apache.org/jira/browse/IGNITE-17838
Implement auto retries
- try {
- tx.rollback(); // Try rolling back on user exception.
- } catch (Exception e) {
- t.addSuppressed(e);
- }
-
- throw t;
- }
+ long startTimestamp = System.currentTimeMillis();
+ long initialTimeout = options == null ?
TimeUnit.SECONDS.toMillis(DEFAULT_RW_TX_TIMEOUT_SECONDS) :
options.timeoutMillis();
Review Comment:
options are nullable
##########
modules/api/src/main/java/org/apache/ignite/tx/IgniteTransactionDefaults.java:
##########
@@ -0,0 +1,28 @@
+/*
+ * 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.tx;
+
+/**
+ * Utility class containing transaction default constants.
+ */
+public class IgniteTransactionDefaults {
Review Comment:
I don't think that we need the class. Please check my comment for
`TransactionConfigurationSchema`.
##########
modules/api/src/main/java/org/apache/ignite/tx/IgniteTransactions.java:
##########
@@ -125,7 +127,9 @@ default CompletableFuture<Transaction> beginAsync() {
* }
* </pre>
*
- * <p>If the closure is executed normally (no exceptions) the transaction
is automatically committed.
+ * <p>If the closure is executed normally (no exceptions) the transaction
is automatically committed. In a case of exception, the
+ * closure will be retried automatically within the transaction timeout,
so it must be pure function. If the transaction timeout
+ * expires before the closure succeeds and transaction is committed, then
the transaction is rolled back.
Review Comment:
What do you mean, how it's possible to roll back already committed
transaction?
##########
modules/api/src/main/java/org/apache/ignite/tx/IgniteTransactions.java:
##########
@@ -257,26 +267,9 @@ default <T> T runInTransaction(Function<Transaction, T>
clo) throws TransactionE
* @throws TransactionException If a transaction can't be finished
successfully.
*/
default <T> T runInTransaction(Function<Transaction, T> clo, @Nullable
TransactionOptions options) throws TransactionException {
- Objects.requireNonNull(clo);
-
- Transaction tx = begin(options);
-
- try {
- T ret = clo.apply(tx);
-
- tx.commit();
-
- return ret;
- } catch (Throwable t) {
- // TODO FIXME https://issues.apache.org/jira/browse/IGNITE-17838
Implement auto retries
- try {
- tx.rollback(); // Try rolling back on user exception.
- } catch (Exception e) {
- t.addSuppressed(e);
- }
-
- throw t;
- }
+ long startTimestamp = System.currentTimeMillis();
Review Comment:
Please add a comment, why it's valid to have non-unique startTimestamp here
in contrast to common transactions beginTimestamp
`HybridTimestamp beginTimestamp = clockService.now(); // Tick to generate
new unique id.`
--
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
