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-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
