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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
