You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "tkalkirill (via GitHub)" <gi...@apache.org> on 2023/01/23 17:03:19 UTC
[GitHub] [ignite-3] tkalkirill opened a new pull request, #1565: IGNITE-18086 Add index building in IncomingSnapshotCopier
tkalkirill opened a new pull request, #1565:
URL: https://github.com/apache/ignite-3/pull/1565
https://issues.apache.org/jira/browse/IGNITE-18086
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1565: IGNITE-18086 Add index building in IncomingSnapshotCopier
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #1565:
URL: https://github.com/apache/ignite-3/pull/1565#discussion_r1084974490
##########
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:
You're right, I'll fix it.
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1565: IGNITE-18086 Add index building in IncomingSnapshotCopier
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #1565:
URL: https://github.com/apache/ignite-3/pull/1565#discussion_r1084963836
##########
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:
Thanks for the tip =), I'll leave it as it is.
--
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
[GitHub] [ignite-3] tkalkirill merged pull request #1565: IGNITE-18086 Add index building in IncomingSnapshotCopier
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill merged PR #1565:
URL: https://github.com/apache/ignite-3/pull/1565
--
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
[GitHub] [ignite-3] rpuch commented on a diff in pull request #1565: IGNITE-18086 Add index building in IncomingSnapshotCopier
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org