[GitHub] [ignite-3] ibessonov commented on a diff in pull request #1605: IGNITE-18523 Destroy RocksDbTableStorage on destroy()

["ibessonov (via GitHub)" <gi...@apache.org>]

ibessonov commented on code in PR #1605:
URL: https://github.com/apache/ignite-3/pull/1605#discussion_r1091800492


##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/HashIndex.java:
##########
@@ -83,4 +83,12 @@ void destroy(int partitionId, WriteBatch writeBatch) throws RocksDBException {
     @Nullable RocksDbHashIndexStorage get(int partitionId) {
         return storages.get(partitionId);
     }
+
+

Review Comment:
   ```suggestion
   
   ```



##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/AbstractMvTableStorageTest.java:
##########
@@ -544,37 +544,30 @@ public void testStartRebalanceForClosedPartition() {
     }
 
     @Test
-    public void testDestroyTableStorage() throws Exception {
+    public void testDestroyTableStorage() {
         MvPartitionStorage mvPartitionStorage = tableStorage.getOrCreateMvPartition(PARTITION_ID);
         HashIndexStorage hashIndexStorage = tableStorage.getOrCreateHashIndex(PARTITION_ID, hashIdx.id());
         SortedIndexStorage sortedIndexStorage = tableStorage.getOrCreateSortedIndex(PARTITION_ID, sortedIdx.id());
 
-        RowId rowId = new RowId(PARTITION_ID);
-
-        TableRow tableRow = tableRow(new TestKey(0, "0"), new TestValue(1, "1"));
-
-        IndexRow hashIndexRow = indexRow(hashIndexStorage.indexDescriptor(), tableRow, rowId);
-        IndexRow sortedIndexRow = indexRow(sortedIndexStorage.indexDescriptor(), tableRow, rowId);
-
-        mvPartitionStorage.runConsistently(() -> {
-            mvPartitionStorage.addWriteCommitted(rowId, tableRow, clock.now());
-
-            hashIndexStorage.put(hashIndexRow);
-
-            sortedIndexStorage.put(sortedIndexRow);
+        List<IgniteTuple3<RowId, TableRow, HybridTimestamp>> rows = List.of(

Review Comment:
   Why does it have to be a list of tuples? Looks ugly.
   Why a single entry wasn't enough?



##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/SortedIndex.java:
##########
@@ -90,6 +90,8 @@ void destroy(int partitionId, WriteBatch writeBatch) throws RocksDBException {
 
     @Override
     public void close() {
+        storages.values().forEach(RocksDbSortedIndexStorage::close);
+
         indexCf.handle().close();

Review Comment:
   Why don't you use "closeAll" here as well?



##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/HashIndex.java:
##########
@@ -83,4 +83,12 @@ void destroy(int partitionId, WriteBatch writeBatch) throws RocksDBException {
     @Nullable RocksDbHashIndexStorage get(int partitionId) {
         return storages.get(partitionId);
     }
+
+
+    /**
+     * Closes all index storages.
+     */
+    void close() {
+        storages.values().forEach(RocksDbHashIndexStorage::close);

Review Comment:
   Why don't you use "closeAll"?



-- 
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