rpuch commented on code in PR #3451:
URL: https://github.com/apache/ignite-3/pull/3451#discussion_r1533334496
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/AbstractMvTableStorageTest.java:
##########
@@ -857,6 +858,53 @@ void testNextRowIdToBuiltAfterRestart() throws Exception {
}
}
+ @Test
+ void testNextRowIdToBuiltAfterRebalance() {
Review Comment:
```suggestion
void testNextRowIdToBuildAfterRebalance() {
```
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/index/sorted/PageMemorySortedIndexStorage.java:
##########
@@ -161,15 +159,17 @@ public PeekCursor<IndexRow> scan(@Nullable
BinaryTuplePrefix lowerBound, @Nullab
SortedIndexRowKey upper = createBound(upperBound, includeUpper);
- return new ScanCursor<IndexRow>(lower, sortedIndexTree) {
+ return new ScanCursor<IndexRow>(lower) {
+ private final BinaryTupleComparator comparator =
indexTree.getBinaryTupleComparator();
Review Comment:
Could it happen that the constructor captures one tree instance, but the
comparator will be taken from another one due to a race? How about taking the
tree instance before creating the scan cursor explicitly and then using it? It
seems that it will require to restore the explicit argument in the constructor
to pass a tree.
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/index/AbstractRocksDbIndexStorage.java:
##########
@@ -221,7 +221,16 @@ String createStorageInfo() {
*
* @throws RocksDBException If failed to delete data.
*/
- public abstract void destroyData(WriteBatch writeBatch) throws
RocksDBException;
+ public final void destroyData(WriteBatch writeBatch) throws
RocksDBException {
+ destroyData0(writeBatch);
+
+ indexMetaStorage.removeNextRowIdToBuild(writeBatch, indexId,
helper.partitionId());
+
+ nextRowIdToBuild = RowId.lowestRowId(helper.partitionId());
Review Comment:
In quite a few places there is that `RowId.lowestRowId(p)`. You always need
to know the context to understand that it's not just the 'lowest possible
RowId', but that the intention is to refer to the 'default next RowId to build
index'. Let's introduce a utility method somewhere that will accept partition
ID and compute this expression. This will make the code more comprehensible.
The method could be named `defaultNextRowIdToBuildIndex()` - a bit clumsy, but
explains the purpose well.
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/AbstractMvTableStorageTest.java:
##########
@@ -857,6 +858,53 @@ void testNextRowIdToBuiltAfterRestart() throws Exception {
}
}
+ @Test
+ void testNextRowIdToBuiltAfterRebalance() {
+ MvPartitionStorage mvPartitionStorage =
getOrCreateMvPartition(PARTITION_ID);
+
+ IndexStorage hashIndexStorage =
tableStorage.getOrCreateIndex(PARTITION_ID, hashIdx);
+ IndexStorage sortedIndexStorage =
tableStorage.getOrCreateIndex(PARTITION_ID, sortedIdx);
+
+ RowId rowId0 = new RowId(PARTITION_ID);
+ RowId rowId1 = new RowId(PARTITION_ID);
+
+ mvPartitionStorage.runConsistently(locker -> {
+ hashIndexStorage.setNextRowIdToBuild(rowId0);
+ sortedIndexStorage.setNextRowIdToBuild(rowId1);
+
+ return null;
+ });
+
+ assertThat(hashIndexStorage.getNextRowIdToBuild(),
is(equalTo(rowId0)));
+ assertThat(sortedIndexStorage.getNextRowIdToBuild(),
is(equalTo(rowId1)));
+
+ assertThat(mvPartitionStorage.flush(), willCompleteSuccessfully());
+
+ // We expect that nextRowIdToBuild will be reverted to its default
value after the rebalance.
+ assertThat(tableStorage.startRebalancePartition(PARTITION_ID),
willCompleteSuccessfully());
+ assertThat(tableStorage.finishRebalancePartition(PARTITION_ID, 100,
100, BYTE_EMPTY_ARRAY), willCompleteSuccessfully());
+
+ assertThat(hashIndexStorage.getNextRowIdToBuild(),
is(equalTo(RowId.lowestRowId(PARTITION_ID))));
Review Comment:
Let's extract `RowId.lowestRowId(PARTITION_ID)` to a variable called
`defaultNextRowIdToBuild`, this will make the code read more easily
##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/index/AbstractRocksDbIndexStorage.java:
##########
@@ -221,7 +221,16 @@ String createStorageInfo() {
*
* @throws RocksDBException If failed to delete data.
*/
- public abstract void destroyData(WriteBatch writeBatch) throws
RocksDBException;
+ public final void destroyData(WriteBatch writeBatch) throws
RocksDBException {
+ destroyData0(writeBatch);
+
+ indexMetaStorage.removeNextRowIdToBuild(writeBatch, indexId,
helper.partitionId());
+
+ nextRowIdToBuild = RowId.lowestRowId(helper.partitionId());
+ }
+
+ /** Methods that needs to be overridden by the inheritors to destroy
implementation specific data. */
+ abstract void destroyData0(WriteBatch writeBatch) throws RocksDBException;
Review Comment:
Please give it a proper name. Names having `0` are already ugly, and it
seems that we can only tolerate them as private methods. If it defines some
kind of an interface, it should have a better name. How about
`destroyIndexData()`?
--
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]
