ascherbakoff commented on code in PR #5209:
URL: https://github.com/apache/ignite-3/pull/5209#discussion_r1980931784
##########
modules/client/src/main/java/org/apache/ignite/internal/client/tx/ClientTransactions.java:
##########
@@ -33,6 +33,8 @@
* Client transactions implementation.
*/
public class ClientTransactions implements IgniteTransactions {
+ private static final int DEFAULT_TIMEOUT = 0;
Review Comment:
I don't think default timeout should be 0.
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/table/ItTableScanTest.java:
##########
@@ -578,10 +585,12 @@ public void testTwiceScanInTransaction() throws Exception
{
public void testScanWithUpperBound() throws Exception {
KeyValueView<Tuple, Tuple> kvView = table.keyValueView();
- BinaryTuplePrefix lowBound = BinaryTuplePrefix.fromBinaryTuple(new
BinaryTuple(1,
- new BinaryTupleBuilder(1).appendInt(5).build()));
- BinaryTuplePrefix upperBound = BinaryTuplePrefix.fromBinaryTuple(new
BinaryTuple(1,
- new BinaryTupleBuilder(1).appendInt(9).build()));
+ BinaryTuplePrefix lowBound = BinaryTuplePrefix.fromBinaryTuple(
Review Comment:
here and below - formatting issues.
avoid reformatting existing code.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -1730,16 +1761,21 @@ protected CompletableFuture<Void> onClose(boolean
intentionallyClose, long scanI
PendingTxPartitionEnlistment enlistment =
tx.enlistedPartition(replicationGrpId);
opFut = enlistment != null ? completeScan(
- tx.id(),
- replicationGrpId,
- scanId,
- th,
- enlistment.primaryNodeConsistentId(),
- intentionallyClose
+ tx.id(),
Review Comment:
again - formatting is broken
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDmlTest.java:
##########
@@ -373,9 +373,12 @@ public void testMergeBatch() {
assertQuery("SELECT count(*) FROM test2 WHERE b =
0").returns(10_000L).check();
- sql("MERGE INTO test2 dst USING test1 src ON dst.a = src.a"
+ var longerTimeoutOptions = new
TransactionOptions().readOnly(false).timeoutMillis(TimeUnit.MINUTES.toMillis(2));
Review Comment:
I think timeouts should be disabled (set to 0) by default in tests and
enabled explicitely in tests which need timeout.
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/InternalTransaction.java:
##########
@@ -117,9 +117,10 @@ void enlist(
* @param executionTimestamp The timestamp is the time when a read-only
transaction is applied to the remote node. The parameter
* is not used for read-write transactions.
* @param full Full state transaction marker.
+ * @param timeoutExceeded Timeout exceeded flag (commit flag must be
{@code false}).
* @return The future.
*/
- CompletableFuture<Void> finish(boolean commit, @Nullable HybridTimestamp
executionTimestamp, boolean full);
+ CompletableFuture<Void> finish(boolean commit, @Nullable HybridTimestamp
executionTimestamp, boolean full, boolean timeoutExceeded);
Review Comment:
I have the same idea. Abort state must be generalized in some way. Separate
ticket is OK.
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/table/ItTableScanTest.java:
##########
@@ -1040,20 +1048,24 @@ private Row createKeyRow(int id) {
private InternalTransaction startTxWithEnlistedPartition(int partId,
boolean readOnly) {
IgniteImpl ignite = unwrapIgniteImpl(CLUSTER.aliveNode());
- InternalTransaction tx = (InternalTransaction)
ignite.transactions().begin(new TransactionOptions().readOnly(readOnly));
+ InternalTransaction tx = (InternalTransaction)
ignite.transactions().begin(
+ new
TransactionOptions().timeoutMillis(10_000).readOnly(readOnly)
Review Comment:
Why is this required ?
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -514,6 +519,18 @@ private <T> CompletableFuture<T> enlistInTx(
}).thenCompose(identity());
}
+ private long getTimeout(InternalTransaction tx) {
Review Comment:
Better to add method
long timeout(long defaultValue)
to internal transaction
--
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
