sanpwc commented on code in PR #5241:
URL: https://github.com/apache/ignite-3/pull/5241#discussion_r1962151319
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -583,8 +587,10 @@ private CompletableFuture<Collection<BinaryRow>>
enlistCursorInTx(
.coordinatorId(tx.coordinatorId())
.build();
- if (primaryReplicaAndConsistencyToken != null) {
- fut = replicaSvc.invoke(primaryReplicaAndConsistencyToken.get1(),
mapFunc.apply(primaryReplicaAndConsistencyToken.get2()));
+ if (enlistment != null) {
+ enlistment.addTableId(tableId);
Review Comment:
I'm not sure that addTableId() calls are placed in all places needed.
First of all, it's not called in some tests, like ItTableScanTest, thus I
guess when we will switch main towards colocation we will get a failure because
addTable isn't called. It might be difficult to debug it. So either add a todo,
or addTable. Latter is better. Similar to what we've discussed in one of your
previous tickets. Of course addTable is only expected if calling
org.apache.ignite.internal.tx.InternalTransaction#enlist with tableId is not
enough.
Besides that I'm not sure that some of addTable calls aren't missing in
InternalTableImpl. It'll be great to have InternalTable unit test that will
ensure that any of InternalTable public method like put, get, contains, etc
does call addTable if new table is touched. Worth mentioning that we doesn't
discuss the necessity of calling addTable in case of read operations - I assume
that for the ease we should call it. We may discuss it though.
--
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
