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