rpuch commented on code in PR #1565:
URL: https://github.com/apache/ignite-3/pull/1565#discussion_r1084880005
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/PartitionAccessImpl.java:
##########
@@ -213,4 +229,8 @@ private TxStateStorage getTxStateStorage(int partitionId) {
return txStateStorage;
}
+
+ private void addToIndexes(TableRow tableRow, RowId rowId) {
+ indexes.get().forEach(index -> index.put(tableRow, rowId));
Review Comment:
It looks like we forgot about deletions. For a deletion, on the sending
side, the `ReadResult.tableRow()` will return `null`, and
`ResponseEntry.rowVersions[i]' will be `null`, so `IncomingSnapshotCopier` on
line 268
```
TableRow tableRow = new TableRow(entry.rowVersions().get(i).rewind());
```
should make `tableRow = null` for a null `rowVersion` buffer, so
`addToIndexes()` will get a `null` `TableRow`, so it should check for null and
do not add anything to an index for a null value (actually, the code in
`PartitionListener` seems to do just this).
##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/raft/snapshot/PartitionAccessImpl.java:
##########
@@ -213,4 +229,8 @@ private TxStateStorage getTxStateStorage(int partitionId) {
return txStateStorage;
}
+
+ private void addToIndexes(TableRow tableRow, RowId rowId) {
+ indexes.get().forEach(index -> index.put(tableRow, rowId));
Review Comment:
Also, we probably need to add a scenario with a DELETE to our IT tests for
this feature. Existing tests only do INSERTs.
##########
modules/table/src/test/java/org/apache/ignite/internal/table/distributed/raft/snapshot/PartitionAccessImplTest.java:
##########
@@ -119,4 +128,56 @@ void
testMinMaxLastAppliedTerm(@InjectConfiguration("mock.tables.foo {}") Tables
assertEquals(15, partitionAccess.minLastAppliedTerm());
assertEquals(20, partitionAccess.maxLastAppliedTerm());
}
+
+ @Test
+ void testAddWrite() {
+ TestMvTableStorage mvTableStorage = new
TestMvTableStorage(tablesConfig.tables().get("foo"), tablesConfig);
+
+ MvPartitionStorage mvPartitionStorage =
mvTableStorage.getOrCreateMvPartition(TEST_PARTITION_ID);
+
+ TableSchemaAwareIndexStorage indexStorage =
mock(TableSchemaAwareIndexStorage.class);
+
+ PartitionAccess partitionAccess = new PartitionAccessImpl(
+ new PartitionKey(UUID.randomUUID(), TEST_PARTITION_ID),
+ mvTableStorage,
+ new TestTxStateTableStorage(),
+ () -> List.of(indexStorage)
+ );
+
+ RowId rowId = new RowId(TEST_PARTITION_ID);
+ TableRow tableRow = mock(TableRow.class);
+ UUID txId = UUID.randomUUID();
+ UUID commitTableId = UUID.randomUUID();
+
+ partitionAccess.addWrite(rowId, tableRow, txId, commitTableId,
TEST_PARTITION_ID);
+
+ verify(mvPartitionStorage, times(1)).addWrite(eq(rowId), eq(tableRow),
eq(txId), eq(commitTableId), eq(TEST_PARTITION_ID));
Review Comment:
Just a comment: `times(1)` is redundant, if we omit it, it will have the
same effect. But it can be used to highlight to the reader that you are
expecting exactly 1 call (not more), this can be useful sometimes.
--
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]
