tkalkirill commented on code in PR #5184:
URL: https://github.com/apache/ignite-3/pull/5184#discussion_r1944183637
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -898,8 +898,12 @@ public CompletableFuture<BinaryRow> get(BinaryRowEx
keyRow, @Nullable InternalTr
}
if (tx.isReadOnly()) {
- return evaluateReadOnlyRecipientNode(partitionId(keyRow),
tx.readTimestamp())
- .thenCompose(recipientNode -> get(keyRow,
tx.readTimestamp(), tx.id(), tx.coordinatorId(), recipientNode));
+ HybridTimestamp readTimestamp = tx.readTimestamp();
+
+ assert readTimestamp != null : "Read timestamp is not set for
read-only transaction";
Review Comment:
How is this possible?
According to the API, this should not happen and the checks should be
somewhere earlier or in the tests.
I suggest removing this check, it looks redundant.
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -995,8 +999,12 @@ public CompletableFuture<List<BinaryRow>>
getAll(Collection<BinaryRowEx> keyRows
if (tx != null && tx.isReadOnly()) {
BinaryRowEx firstRow = keyRows.iterator().next();
- return evaluateReadOnlyRecipientNode(partitionId(firstRow),
tx.readTimestamp())
- .thenCompose(recipientNode -> getAll(keyRows,
tx.readTimestamp(), tx.id(), tx.coordinatorId(), recipientNode));
+ HybridTimestamp readTimestamp = tx.readTimestamp();
+
+ assert readTimestamp != null : "Read timestamp is not set for
read-only transaction";
Review Comment:
Same
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -2098,14 +2106,24 @@ protected CompletableFuture<ClusterNode>
evaluateReadOnlyRecipientNode(int partI
throw withCause(TransactionException::new,
REPLICA_UNAVAILABLE_ERR, e);
} else {
if (res == null) {
- throw withCause(TransactionException::new,
REPLICA_UNAVAILABLE_ERR, e);
+ throw createTransactionException(tablePartitionId,
readTimestamp);
} else {
return getClusterNode(res);
}
}
});
}
+ private static TransactionException
createTransactionException(TablePartitionId tablePartitionId, HybridTimestamp
readTimestamp) {
Review Comment:
A very general name for a method, it can be misused, it is better to give it
a name like "createFailedGetPromaryReplicaTransactionException".
--
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]
