[GitHub] [ignite-3] ibessonov commented on a diff in pull request #1745: IGNITE-18529 Deal with deleting a table/partition and rebalancing partitions

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

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


##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/util/MvPartitionStorages.java:
##########
@@ -442,6 +403,8 @@ private void throwExceptionDependingOnOperation(StorageOperation operation, int
             throw new StorageRebalanceException(createStorageInProgressOfFinishRebalanceErrorMessage(partitionId));
         } else if (operation instanceof CleanupStorageOperation) {
             throw new StorageException(createStorageInProgressOfCleanupErrorMessage(partitionId));
+        } else if (operation instanceof CloseStorageOperation || operation.isFinalOperation()) {

Review Comment:
   I am glad that there are individual classes for different operations.
   But, I am not glad that you have a bunch of `if-else` statements instead of using polymorphism.
   What's the point of having classes then?



##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/util/StorageOperation.java:
##########
@@ -25,16 +25,55 @@
  * Storage operations.
  */
 interface StorageOperation {
+    /**
+     * Returns future completion of the operation.
+     */
+    CompletableFuture<Void> operationFuture();
+
+    /**
+     * Return {@code true} if the operation is the final.
+     */
+    boolean isFinalOperation();
+
+    /**
+     * Marks the operation as final.
+     */
+    void markFinalOperation();
+
+    /**
+     * Abstract operation of the storage.
+     */
+    abstract class AbstractStorageOperation implements StorageOperation {
+        private final CompletableFuture<Void> operationFuture = new CompletableFuture<>();
+
+        private volatile boolean finalOperation;
+
+        @Override
+        public CompletableFuture<Void> operationFuture() {
+            return operationFuture;
+        }
+
+        @Override
+        public boolean isFinalOperation() {
+            return finalOperation;
+        }
+
+        @Override
+        public void markFinalOperation() {
+            finalOperation = true;
+        }
+    }
+
     /**
      * Storage creation operation.
      */
-    class CreateStorageOperation implements StorageOperation {
+    class CreateStorageOperation extends AbstractStorageOperation {

Review Comment:
   Why are all these classes implemented as internal classes? You can move them to corresponding package.
   By the way, why is it located in the "util"?



##########
modules/storage-rocksdb/src/main/java/org/apache/ignite/internal/storage/rocksdb/RocksDbTableStorage.java:
##########
@@ -358,19 +359,22 @@ public void stop() throws StorageException {
 
         resources.add(writeOptions);
 
-        mvPartitionStorages.getAllForClose().forEach(mvPartitionStorage -> resources.add(mvPartitionStorage::close));
+        try {
+            mvPartitionStorages
+                    .getAllForCloseOrDestroy()
+                    .get(10, TimeUnit.SECONDS)

Review Comment:
   Same here, please add a comment, why 10 seconds is enough



##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/AbstractPageMemoryTableStorage.java:
##########
@@ -103,7 +105,7 @@ public void stop() throws StorageException {
         busyLock.block();
 
         try {
-            IgniteUtils.closeAll(mvPartitionStorages.getAllForClose().stream().map(mvPartitionStorage -> mvPartitionStorage::close));
+            IgniteUtils.closeAllManually(mvPartitionStorages.getAllForCloseOrDestroy().get(10, TimeUnit.SECONDS).stream());

Review Comment:
   Maybe extract a future into a variable, so the code will not look as tight



##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/util/StorageOperation.java:
##########
@@ -25,16 +25,55 @@
  * Storage operations.
  */
 interface StorageOperation {
+    /**
+     * Returns future completion of the operation.
+     */
+    CompletableFuture<Void> operationFuture();
+
+    /**
+     * Return {@code true} if the operation is the final.
+     */
+    boolean isFinalOperation();
+
+    /**
+     * Marks the operation as final.
+     */
+    void markFinalOperation();
+
+    /**
+     * Abstract operation of the storage.
+     */
+    abstract class AbstractStorageOperation implements StorageOperation {

Review Comment:
   If all implementations must extend abstract class now, why do you need an interface? Just replace it with abstract class.



##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/AbstractPageMemoryTableStorage.java:
##########
@@ -103,7 +105,7 @@ public void stop() throws StorageException {
         busyLock.block();
 
         try {
-            IgniteUtils.closeAll(mvPartitionStorages.getAllForClose().stream().map(mvPartitionStorage -> mvPartitionStorage::close));
+            IgniteUtils.closeAllManually(mvPartitionStorages.getAllForCloseOrDestroy().get(10, TimeUnit.SECONDS).stream());

Review Comment:
   Please add a comment about 10 seconds, explaining why it's enough.



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