You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "vldpyatkov (via GitHub)" <gi...@apache.org> on 2023/09/01 14:49:20 UTC
[GitHub] [ignite-3] vldpyatkov opened a new pull request, #2537: IGNITE-20322 POC
vldpyatkov opened a new pull request, #2537:
URL: https://github.com/apache/ignite-3/pull/2537
(no comment)
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20322 Add ability to pass an observable timestamp to an implicit transaction [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #2537:
URL: https://github.com/apache/ignite-3/pull/2537#discussion_r1833956830
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/TransactionIds.java:
##########
@@ -34,8 +37,8 @@ public class TransactionIds {
* @param priority Transaction priority.
* @return Transaction ID corresponding to the provided values.
*/
- public static UUID transactionId(HybridTimestamp beginTimestamp, int nodeId, TxPriority priority) {
- return transactionId(beginTimestamp.longValue(), nodeId, priority);
+ public static UUID transactionId(HybridTimestamp beginTimestamp, int nodeId, boolean implicit, TxPriority priority) {
Review Comment:
I believe adding implicit flag to txId is wrong.
1. It's not related to ticket description.
2. It has no use in the product
3. It affects future backward compatibility.
Instead, it should be moved to internal tx state field:
`private boolean implicit;`
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20322 Add ability to pass an observable timestamp to an implicit transaction [ignite-3]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2537:
URL: https://github.com/apache/ignite-3/pull/2537#discussion_r1832224483
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientObservationTsTest.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.runner.app.client;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import org.apache.ignite.table.Table;
+import org.apache.ignite.table.Tuple;
+import org.apache.ignite.tx.Transaction;
+import org.apache.ignite.tx.TransactionOptions;
+import org.junit.jupiter.api.Test;
+
+/**
+ * This test is checking correctness of observation timestamp calculation.
+ */
+public class ItThinClientObservationTsTest extends ItAbstractThinClientTest {
+
+ @Test
+ public void test() {
+ Table srvTable = server().tables().table(TABLE_NAME);
+ Table clientTable = client().tables().table(TABLE_NAME);
+
+ log.info("Tables [client={}, srv={}]", clientTable.getClass().getSimpleName(), srvTable.getClass().getSimpleName());
+
+ srvTable.keyValueView().put(null, Tuple.create().set(COLUMN_KEY, 42), Tuple.create().set(COLUMN_VAL, "srv value"));
+
+ clientTable.keyValueView().put(null, Tuple.create().set(COLUMN_KEY, 42), Tuple.create().set(COLUMN_VAL, "client value"));
+
+ Transaction tx = client().transactions().begin(new TransactionOptions().readOnly(true));
+
+ String clientValue = clientTable.keyValueView().get(tx, Tuple.create().set(COLUMN_KEY, 42)).value(COLUMN_VAL);
+
+ tx.commit();
+
+ Transaction srvTx = server().transactions().begin(new TransactionOptions().readOnly(true));
Review Comment:
plz configure _idleSafeTimePropagationDuration_ before this and the same tests, for avoid potential flaky on jvm freeze
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20322 Add ability to pass an observable timestamp to an implicit transaction [ignite-3]
Posted by "vldpyatkov (via GitHub)" <gi...@apache.org>.
vldpyatkov commented on code in PR #2537:
URL: https://github.com/apache/ignite-3/pull/2537#discussion_r1837155292
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/requests/table/ClientTupleDeleteAllExactRequest.java:
##########
@@ -35,20 +36,22 @@ public class ClientTupleDeleteAllExactRequest {
/**
* Processes the request.
*
- * @param in Unpacker.
- * @param out Packer.
- * @param tables Ignite tables.
+ * @param in Unpacker.
+ * @param out Packer.
+ * @param tables Ignite tables.
* @param resources Resource registry.
+ * @param igniteTransactions Ignite transactions.
* @return Future.
*/
public static CompletableFuture<Void> process(
ClientMessageUnpacker in,
ClientMessagePacker out,
IgniteTables tables,
- ClientResourceRegistry resources
+ ClientResourceRegistry resources,
+ IgniteTransactionsImpl igniteTransactions
) {
return readTableAsync(in, tables).thenCompose(table -> {
- var tx = readTx(in, out, resources);
+ var tx = readOrStartImplicitTx(in, out, resources, igniteTransactions, false);
Review Comment:
I fixed it in all places in the client handler where we are starting implicit transactions now.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20322 Add ability to pass an observable timestamp to an implicit transaction [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #2537:
URL: https://github.com/apache/ignite-3/pull/2537#discussion_r1833956830
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/TransactionIds.java:
##########
@@ -34,8 +37,8 @@ public class TransactionIds {
* @param priority Transaction priority.
* @return Transaction ID corresponding to the provided values.
*/
- public static UUID transactionId(HybridTimestamp beginTimestamp, int nodeId, TxPriority priority) {
- return transactionId(beginTimestamp.longValue(), nodeId, priority);
+ public static UUID transactionId(HybridTimestamp beginTimestamp, int nodeId, boolean implicit, TxPriority priority) {
Review Comment:
I believe adding implicit flag to txId is wrong.
1. It's not related to ticket description.
2. It has no use for the product
3. It affects future backward compatibility.
Instead, it should be moved to internal tx state field:
`private boolean implicit;`
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/requests/table/ClientTupleGetAllRequest.java:
##########
@@ -49,6 +49,7 @@ public static CompletableFuture<Void> process(
ClientResourceRegistry resources
) {
return readTableAsync(in, tables).thenCompose(table -> {
+ // TODO: IGNITE-23603 We have to create an implicit transaction, but leave a possibility to start RO direct.
Review Comment:
Do you mean we currently using implicit RW tx here, but should use RO direct instead ?
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/ReadWriteTransactionImpl.java:
##########
@@ -135,9 +140,24 @@ private void checkEnlistPossibility() {
}
}
- /** {@inheritDoc} */
@Override
- protected CompletableFuture<Void> finish(boolean commit) {
+ public CompletableFuture<Void> commitAsync() {
+ return TransactionsExceptionMapperUtil.convertToPublicFuture(
+ finish(true, nullableHybridTimestamp(NULL_HYBRID_TIMESTAMP), false),
+ TX_COMMIT_ERR
+ );
+ }
+
+ @Override
+ public CompletableFuture<Void> rollbackAsync() {
+ return TransactionsExceptionMapperUtil.convertToPublicFuture(
+ finish(false, nullableHybridTimestamp(NULL_HYBRID_TIMESTAMP), false),
+ TX_ROLLBACK_ERR
+ );
+ }
+
+ @Override
+ public CompletableFuture<Void> finish(boolean commit, HybridTimestamp executionTimestamp, boolean full) {
Review Comment:
Why execution timestamp is not nullable?
This is to avoid constructing useless object.
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientInboundMessageHandler.java:
##########
@@ -661,49 +661,49 @@ private void processOperation(ChannelHandlerContext ctx, ClientMessageUnpacker i
return ClientTableGetRequest.process(in, out, igniteTables);
case ClientOp.TUPLE_UPSERT:
- return ClientTupleUpsertRequest.process(in, out, igniteTables, resources);
+ return ClientTupleUpsertRequest.process(in, out, igniteTables, resources, igniteTransactions);
Review Comment:
We don't need to track a client observable ts on server because it's already tracked on client for RO purposes:
`org.apache.ignite.internal.client.ReliableChannel#observableTimestamp`
This should be fixed. Instead, the commit ts should be propagated directly to client on commit.
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/IgniteTransactionsImpl.java:
##########
@@ -103,8 +103,18 @@ public CompletableFuture<Transaction> beginAsync(@Nullable TransactionOptions op
return CompletableFuture.completedFuture(begin(options));
}
+ /**
+ * Begins a transaction.
+ *
+ * @param readOnly {@code true} in order to start a read-only transaction, {@code false} in order to start read-write one.
+ * @return The started transaction.
+ */
+ public InternalTransaction beginImplicit(boolean readOnly) {
Review Comment:
I believe it's not OK to add implicit flag to a transaction state as the part of this PR.
This introduces a log of unrelated bloating changes and better to be done as separate PR.
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/requests/table/ClientTupleDeleteAllExactRequest.java:
##########
@@ -35,20 +36,22 @@ public class ClientTupleDeleteAllExactRequest {
/**
* Processes the request.
*
- * @param in Unpacker.
- * @param out Packer.
- * @param tables Ignite tables.
+ * @param in Unpacker.
+ * @param out Packer.
+ * @param tables Ignite tables.
* @param resources Resource registry.
+ * @param igniteTransactions Ignite transactions.
* @return Future.
*/
public static CompletableFuture<Void> process(
ClientMessageUnpacker in,
ClientMessagePacker out,
IgniteTables tables,
- ClientResourceRegistry resources
+ ClientResourceRegistry resources,
+ IgniteTransactionsImpl igniteTransactions
) {
return readTableAsync(in, tables).thenCompose(table -> {
- var tx = readTx(in, out, resources);
+ var tx = readOrStartImplicitTx(in, out, resources, igniteTransactions, false);
Review Comment:
Looks like commit timestamp is not propagated to the client's obervable ts in out.meta field.
Instead, client's observable ts is set to server hlc, which is incorrect.
This affects all client RW update requests and commit request and should be fixed.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20322 Add ability to pass an observable timestamp to an implicit transaction [ignite-3]
Posted by "denis-chudov (via GitHub)" <gi...@apache.org>.
denis-chudov commented on code in PR #2537:
URL: https://github.com/apache/ignite-3/pull/2537#discussion_r1831500153
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/TransactionIds.java:
##########
@@ -76,14 +80,22 @@ public static int nodeId(UUID transactionId) {
}
public static TxPriority priority(UUID txId) {
- int ordinal = (int) (txId.getLeastSignificantBits() & 1);
+ int ordinal = (int) ((txId.getLeastSignificantBits() >> 16) & 1);
return TxPriority.values()[ordinal];
}
- private static long combine(int nodeId, TxPriority priority) {
- int priorityAsInt = priority.ordinal();
+ public static boolean implicit(UUID txId) {
+ return (txId.getLeastSignificantBits() & IMPLICIT_TX_FLAG) == 1;
+ }
+
+ private static long combine(int nodeId, TxPriority priority, boolean implicit) {
+ int priorityAndFlagsAsInt = priority.ordinal() << 16;
Review Comment:
Let's introduce `16` as a constant offset for priority.
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/tx/QueryTransactionWrapperImpl.java:
##########
@@ -28,7 +28,11 @@
* Wrapper for the transaction that encapsulates the management of an implicit transaction.
*/
public class QueryTransactionWrapperImpl implements QueryTransactionWrapper {
- private final boolean implicit;
+ /**
+ * This flag does not match with the implicit transact.
Review Comment:
```suggestion
* This flag does not signify the type of transaction.
```
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/TxManager.java:
##########
@@ -52,29 +53,29 @@ public interface TxManager extends IgniteComponent {
* @param timestampTracker Observable timestamp tracker is used to track a timestamp for either read-write or read-only
* transaction execution. The tracker is also used to determine the read timestamp for read-only transactions. Each client
* should pass its own tracker to provide linearizability between read-write and read-only transactions started by this client.
+ * @param implicit Either the transaction is implicit or not.
* @param readOnly {@code true} in order to start a read-only transaction, {@code false} in order to start read-write one.
* Calling begin with readOnly {@code false} is an equivalent of TxManager#begin().
* @return The started transaction.
* @throws IgniteInternalException with {@link Transactions#TX_READ_ONLY_TOO_OLD_ERR} if transaction much older than the data
* available in the tables.
*/
- InternalTransaction begin(HybridTimestampTracker timestampTracker, boolean readOnly);
+ InternalTransaction begin(HybridTimestampTracker timestampTracker, boolean implicit, boolean readOnly);
/**
* Starts either read-write or read-only transaction, depending on {@code readOnly} parameter value.
*
* @param timestampTracker Observable timestamp tracker is used to track a timestamp for either read-write or read-only
* transaction execution. The tracker is also used to determine the read timestamp for read-only transactions. Each client
* should pass its own tracker to provide linearizability between read-write and read-only transactions started by this client.
+ * @param implicit Either the transaction is implicit or not.
Review Comment:
```suggestion
* @param implicit Whether the transaction is implicit or not.
```
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/TxManager.java:
##########
@@ -41,9 +41,10 @@ public interface TxManager extends IgniteComponent {
*
* @param timestampTracker Observable timestamp tracker is used to track a timestamp for either read-write or read-only
* transaction execution. The tracker is also used to determine the read timestamp for read-only transactions.
+ * @param implicit Either the transaction is implicit or not.
Review Comment:
```suggestion
* @param implicit Whether the transaction is implicit or not.
```
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/TxManager.java:
##########
@@ -52,29 +53,29 @@ public interface TxManager extends IgniteComponent {
* @param timestampTracker Observable timestamp tracker is used to track a timestamp for either read-write or read-only
* transaction execution. The tracker is also used to determine the read timestamp for read-only transactions. Each client
* should pass its own tracker to provide linearizability between read-write and read-only transactions started by this client.
+ * @param implicit Either the transaction is implicit or not.
Review Comment:
```suggestion
* @param implicit Whether the transaction is implicit or not.
```
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/TransactionIds.java:
##########
@@ -55,10 +58,11 @@ public static UUID transactionId(HybridTimestamp beginTimestamp, int nodeId) {
* @param beginTimestamp Transaction begin timestamp.
* @param nodeId Unique ID of the current node used to make generated transaction IDs globally unique.
* @param priority Transaction priority.
+ * @param implicit Transaction is implicitly started or not.
Review Comment:
```suggestion
* @param implicit Whether the transaction is started implicitly or not.
```
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/TransactionIds.java:
##########
@@ -26,6 +26,9 @@
* Collection of utils to generate and pick apart transaction IDs.
*/
public class TransactionIds {
+
+ public static final int IMPLICIT_TX_FLAG = 0x00000001;
Review Comment:
can be private.
And why not just `0x1`?
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20322 Add ability to pass an observable timestamp to an implicit transaction [ignite-3]
Posted by "vldpyatkov (via GitHub)" <gi...@apache.org>.
vldpyatkov commented on code in PR #2537:
URL: https://github.com/apache/ignite-3/pull/2537#discussion_r1836633048
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/requests/table/ClientTupleGetAllRequest.java:
##########
@@ -49,6 +49,7 @@ public static CompletableFuture<Void> process(
ClientResourceRegistry resources
) {
return readTableAsync(in, tables).thenCompose(table -> {
+ // TODO: IGNITE-23603 We have to create an implicit transaction, but leave a possibility to start RO direct.
Review Comment:
Currently, we can start a direct RO transaction only if the null value is passed.
In the tiket IGNITE-23603 we will provide API to start a direct 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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20322 Add ability to pass an observable timestamp to an implicit transaction [ignite-3]
Posted by "vldpyatkov (via GitHub)" <gi...@apache.org>.
vldpyatkov merged PR #2537:
URL: https://github.com/apache/ignite-3/pull/2537
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20322 Add ability to pass an observable timestamp to an implicit transaction [ignite-3]
Posted by "vldpyatkov (via GitHub)" <gi...@apache.org>.
vldpyatkov commented on code in PR #2537:
URL: https://github.com/apache/ignite-3/pull/2537#discussion_r1837153662
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/TransactionIds.java:
##########
@@ -34,8 +37,8 @@ public class TransactionIds {
* @param priority Transaction priority.
* @return Transaction ID corresponding to the provided values.
*/
- public static UUID transactionId(HybridTimestamp beginTimestamp, int nodeId, TxPriority priority) {
- return transactionId(beginTimestamp.longValue(), nodeId, priority);
+ public static UUID transactionId(HybridTimestamp beginTimestamp, int nodeId, boolean implicit, TxPriority priority) {
Review Comment:
I changed it to prived field in transaction object.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20322 Add ability to pass an observable timestamp to an implicit transaction [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #2537:
URL: https://github.com/apache/ignite-3/pull/2537#discussion_r1834007776
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/IgniteTransactionsImpl.java:
##########
@@ -103,8 +103,18 @@ public CompletableFuture<Transaction> beginAsync(@Nullable TransactionOptions op
return CompletableFuture.completedFuture(begin(options));
}
+ /**
+ * Begins a transaction.
+ *
+ * @param readOnly {@code true} in order to start a read-only transaction, {@code false} in order to start read-write one.
+ * @return The started transaction.
+ */
+ public InternalTransaction beginImplicit(boolean readOnly) {
Review Comment:
I believe it's not OK to add implicit flag to a transaction state as the part of this PR.
This introduces a lot of unrelated bloating changes and better to be done as separate PR.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20322 Add ability to pass an observable timestamp to an implicit transaction [ignite-3]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2537:
URL: https://github.com/apache/ignite-3/pull/2537#discussion_r1832225100
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientObservationTsTest.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.runner.app.client;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import org.apache.ignite.table.Table;
+import org.apache.ignite.table.Tuple;
+import org.apache.ignite.tx.Transaction;
+import org.apache.ignite.tx.TransactionOptions;
+import org.junit.jupiter.api.Test;
+
+/**
+ * This test is checking correctness of observation timestamp calculation.
+ */
+public class ItThinClientObservationTsTest extends ItAbstractThinClientTest {
+
+ @Test
+ public void test() {
+ Table srvTable = server().tables().table(TABLE_NAME);
+ Table clientTable = client().tables().table(TABLE_NAME);
+
+ log.info("Tables [client={}, srv={}]", clientTable.getClass().getSimpleName(), srvTable.getClass().getSimpleName());
Review Comment:
remove or change log string
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20322 Add ability to pass an observable timestamp to an implicit transaction [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #2537:
URL: https://github.com/apache/ignite-3/pull/2537#discussion_r1837851070
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/IgniteTransactionsImpl.java:
##########
@@ -103,8 +103,18 @@ public CompletableFuture<Transaction> beginAsync(@Nullable TransactionOptions op
return CompletableFuture.completedFuture(begin(options));
}
+ /**
+ * Begins a transaction.
+ *
+ * @param readOnly {@code true} in order to start a read-only transaction, {@code false} in order to start read-write one.
+ * @return The started transaction.
+ */
+ public InternalTransaction beginImplicit(boolean readOnly) {
Review Comment:
All beginXXX methods alredy accept HybridTimestampTracker as an argument.
To fix the issue in the ticket description it's not necessary to add `implicit `state field to a 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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20322 Add ability to pass an observable timestamp to an implicit transaction [ignite-3]
Posted by "vldpyatkov (via GitHub)" <gi...@apache.org>.
vldpyatkov commented on code in PR #2537:
URL: https://github.com/apache/ignite-3/pull/2537#discussion_r1837160051
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/IgniteTransactionsImpl.java:
##########
@@ -103,8 +103,18 @@ public CompletableFuture<Transaction> beginAsync(@Nullable TransactionOptions op
return CompletableFuture.completedFuture(begin(options));
}
+ /**
+ * Begins a transaction.
+ *
+ * @param readOnly {@code true} in order to start a read-only transaction, {@code false} in order to start read-write one.
+ * @return The started transaction.
+ */
+ public InternalTransaction beginImplicit(boolean readOnly) {
Review Comment:
The possibility to start an implicit transaction in another component is a main cause of this PR. Without this, we cannot handle an observable timestamp.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20322 Add ability to pass an observable timestamp to an implicit transaction [ignite-3]
Posted by "vldpyatkov (via GitHub)" <gi...@apache.org>.
vldpyatkov commented on code in PR #2537:
URL: https://github.com/apache/ignite-3/pull/2537#discussion_r1837005862
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/ReadWriteTransactionImpl.java:
##########
@@ -135,9 +140,24 @@ private void checkEnlistPossibility() {
}
}
- /** {@inheritDoc} */
@Override
- protected CompletableFuture<Void> finish(boolean commit) {
+ public CompletableFuture<Void> commitAsync() {
+ return TransactionsExceptionMapperUtil.convertToPublicFuture(
+ finish(true, nullableHybridTimestamp(NULL_HYBRID_TIMESTAMP), false),
+ TX_COMMIT_ERR
+ );
+ }
+
+ @Override
+ public CompletableFuture<Void> rollbackAsync() {
+ return TransactionsExceptionMapperUtil.convertToPublicFuture(
+ finish(false, nullableHybridTimestamp(NULL_HYBRID_TIMESTAMP), false),
+ TX_ROLLBACK_ERR
+ );
+ }
+
+ @Override
+ public CompletableFuture<Void> finish(boolean commit, HybridTimestamp executionTimestamp, boolean full) {
Review Comment:
Done.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20322 Add ability to pass an observable timestamp to an implicit transaction [ignite-3]
Posted by "vldpyatkov (via GitHub)" <gi...@apache.org>.
vldpyatkov commented on code in PR #2537:
URL: https://github.com/apache/ignite-3/pull/2537#discussion_r1836635606
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/TransactionIds.java:
##########
@@ -34,8 +37,8 @@ public class TransactionIds {
* @param priority Transaction priority.
* @return Transaction ID corresponding to the provided values.
*/
- public static UUID transactionId(HybridTimestamp beginTimestamp, int nodeId, TxPriority priority) {
- return transactionId(beginTimestamp.longValue(), nodeId, priority);
+ public static UUID transactionId(HybridTimestamp beginTimestamp, int nodeId, boolean implicit, TxPriority priority) {
Review Comment:
Fortunately, at this moment we do not have to bother about compatibility, but I will try to move the flag out of transaction 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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20322 Add ability to pass an observable timestamp to an implicit transaction [ignite-3]
Posted by "vldpyatkov (via GitHub)" <gi...@apache.org>.
vldpyatkov commented on code in PR #2537:
URL: https://github.com/apache/ignite-3/pull/2537#discussion_r1837152689
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/requests/table/ClientTupleGetAllRequest.java:
##########
@@ -49,6 +49,7 @@ public static CompletableFuture<Void> process(
ClientResourceRegistry resources
) {
return readTableAsync(in, tables).thenCompose(table -> {
+ // TODO: IGNITE-23603 We have to create an implicit transaction, but leave a possibility to start RO direct.
Review Comment:
I thought again, and currently we still have this issue only in batch get operation.
We have to determine which transaction has to be started, RO or RW. It depends on if all keys are in one partition or not.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20322 Add ability to pass an observable timestamp to an implicit transaction [ignite-3]
Posted by "vldpyatkov (via GitHub)" <gi...@apache.org>.
vldpyatkov commented on PR #2537:
URL: https://github.com/apache/ignite-3/pull/2537#issuecomment-2456724134
Incorrect TC runs were attached.
https://ci.ignite.apache.org/buildConfiguration/ApacheIgnite3xGradle_Test_RunAllTests?branch=pull%2F2537&buildTypeTab=overview&mode=builds
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20322 Add ability to pass an observable timestamp to an implicit transaction [ignite-3]
Posted by "vldpyatkov (via GitHub)" <gi...@apache.org>.
vldpyatkov commented on code in PR #2537:
URL: https://github.com/apache/ignite-3/pull/2537#discussion_r1837158107
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/ClientInboundMessageHandler.java:
##########
@@ -661,49 +661,49 @@ private void processOperation(ChannelHandlerContext ctx, ClientMessageUnpacker i
return ClientTableGetRequest.process(in, out, igniteTables);
case ClientOp.TUPLE_UPSERT:
- return ClientTupleUpsertRequest.process(in, out, igniteTables, resources);
+ return ClientTupleUpsertRequest.process(in, out, igniteTables, resources, igniteTransactions);
Review Comment:
Currently, we can't do that. Probably we might think about this later.
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20322 Add ability to pass an observable timestamp to an implicit transaction [ignite-3]
Posted by "ascherbakoff (via GitHub)" <gi...@apache.org>.
ascherbakoff commented on code in PR #2537:
URL: https://github.com/apache/ignite-3/pull/2537#discussion_r1837851070
##########
modules/transactions/src/main/java/org/apache/ignite/internal/tx/impl/IgniteTransactionsImpl.java:
##########
@@ -103,8 +103,18 @@ public CompletableFuture<Transaction> beginAsync(@Nullable TransactionOptions op
return CompletableFuture.completedFuture(begin(options));
}
+ /**
+ * Begins a transaction.
+ *
+ * @param readOnly {@code true} in order to start a read-only transaction, {@code false} in order to start read-write one.
+ * @return The started transaction.
+ */
+ public InternalTransaction beginImplicit(boolean readOnly) {
Review Comment:
All beginXXX methods already accept HybridTimestampTracker as an argument.
To fix the issue in the ticket description it's not necessary to add `implicit `state field to a 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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org