tkalkirill commented on code in PR #6033:
URL: https://github.com/apache/ignite-3/pull/6033#discussion_r2149568986


##########
modules/distribution-zones/src/integrationTest/java/org/apache/ignite/internal/rebalance/ItRebalanceTest.java:
##########
@@ -146,6 +149,34 @@ void assignmentsChangingOnNodeLeaveNodeJoin() throws 
Exception {
         );
     }
 
+    @Test
+    @WithSystemProperty(key = THREAD_ASSERTIONS_ENABLED, value = "false")
+    public void compactionAfterAbortedRebalance() {
+        createZone("TEST_ZONE", 1, 1);
+        createTestTable("TEST_TABLE", "TEST_ZONE");
+
+        IgniteImpl igniteImpl = igniteImpl(0);
+
+        TableViewInternal table = 
unwrapTableViewInternal(igniteImpl.tables().table("TEST_TABLE"));
+
+        MvTableStorage storage = table.internalTable().storage();
+
+        table.keyValueView().put(
+                null,
+                Tuple.create().set("id", 1),
+                Tuple.create().set("val", "value1")
+        );
+
+        CompletableFuture<CompletableFuture<Void>> abortRebalance = 
storage.startRebalancePartition(0)

Review Comment:
   According to the documentation, if the rebalance is not in progress, then 
nothing will happen. Let's say after the rebalance starts, some internal 
component will finish it for some reason. What do you expect after that?
   I suggest at least writing a comment about it.



##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/PersistentPageMemoryTableStorage.java:
##########
@@ -353,13 +356,40 @@ CompletableFuture<Void> 
clearStorageAndUpdateDataStructures(AbstractPageMemoryMv
     }
 
     private CompletableFuture<Void> 
destroyPartitionPhysically(GroupPartitionId groupPartitionId) {
-        
dataRegion.filePageStoreManager().getStore(groupPartitionId).markToDestroy();
+        FilePageStore store = 
dataRegion.filePageStoreManager().getStore(groupPartitionId);
 
-        dataRegion.pageMemory().invalidate(groupPartitionId.getGroupId(), 
groupPartitionId.getPartitionId());
+        assert store != null : groupPartitionId;
 
-        return 
dataRegion.checkpointManager().onPartitionDestruction(groupPartitionId)
-                .thenAccept(unused -> 
dataRegion.partitionMetaManager().removeMeta(groupPartitionId))
-                .thenCompose(unused -> 
dataRegion.filePageStoreManager().destroyPartition(groupPartitionId));
+        store.markToDestroy();
+
+        CompletableFuture<Void> future = new CompletableFuture<>();
+
+        CheckpointManager checkpointManager = dataRegion.checkpointManager();
+
+        CheckpointListener listener = new CheckpointListener() {
+            @Override
+            public void afterCheckpointEnd(CheckpointProgress progress) {
+                checkpointManager.removeCheckpointListener(this);
+
+                try {
+                    
dataRegion.pageMemory().invalidate(groupPartitionId.getGroupId(), 
groupPartitionId.getPartitionId());
+
+                    
dataRegion.partitionMetaManager().removeMeta(groupPartitionId);
+
+                    future.complete(null);
+                } catch (Exception e) {
+                    future.completeExceptionally(new 
StorageException("Couldn't destroy partition: " + groupPartitionId, e));
+                }
+            }
+        };
+
+        checkpointManager.addCheckpointListener(listener, dataRegion);
+
+        CheckpointProgress checkpoint = 
dataRegion.checkpointManager().forceCheckpoint("Partition destruction");

Review Comment:
   Will there be problems here, for example, when rebalancing multiple 
partitions? as a result of which we will initiate many checkpoints, which will 
be unnecessary and can cause node degradation?



##########
modules/distribution-zones/src/integrationTest/java/org/apache/ignite/internal/rebalance/ItRebalanceTest.java:
##########
@@ -146,6 +149,34 @@ void assignmentsChangingOnNodeLeaveNodeJoin() throws 
Exception {
         );
     }
 
+    @Test
+    @WithSystemProperty(key = THREAD_ASSERTIONS_ENABLED, value = "false")
+    public void compactionAfterAbortedRebalance() {
+        createZone("TEST_ZONE", 1, 1);
+        createTestTable("TEST_TABLE", "TEST_ZONE");
+
+        IgniteImpl igniteImpl = igniteImpl(0);
+
+        TableViewInternal table = 
unwrapTableViewInternal(igniteImpl.tables().table("TEST_TABLE"));
+
+        MvTableStorage storage = table.internalTable().storage();
+
+        table.keyValueView().put(
+                null,
+                Tuple.create().set("id", 1),
+                Tuple.create().set("val", "value1")
+        );
+
+        CompletableFuture<CompletableFuture<Void>> abortRebalance = 
storage.startRebalancePartition(0)
+                .thenApply(v -> storage.abortRebalancePartition(0));
+
+        assertThat(abortRebalance, willCompleteSuccessfully());
+
+        CompletableFuture<Void> flush = storage.getMvPartition(0).flush(true);
+
+        assertThat(flush, willCompleteSuccessfully());

Review Comment:
   I think that inline of future are enough here.
   According to the name of the test, you want to check compaction, but you 
only check flash, I think something in the test is not finished yet.



##########
modules/distribution-zones/src/integrationTest/java/org/apache/ignite/internal/rebalance/ItRebalanceTest.java:
##########
@@ -146,6 +149,34 @@ void assignmentsChangingOnNodeLeaveNodeJoin() throws 
Exception {
         );
     }
 
+    @Test
+    @WithSystemProperty(key = THREAD_ASSERTIONS_ENABLED, value = "false")
+    public void compactionAfterAbortedRebalance() {
+        createZone("TEST_ZONE", 1, 1);

Review Comment:
   Maybe we can move the names of zones and tables to constants?



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

Reply via email to