tkalkirill commented on code in PR #3352:
URL: https://github.com/apache/ignite-3/pull/3352#discussion_r1512870862
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/AbstractPageMemoryMvPartitionStorage.java:
##########
@@ -969,4 +977,44 @@ IndexMeta createIndexMetaForNewIndex(int indexId) {
return sortedIndexes.get(indexId);
});
}
+
+ /**
+ * Destroys an index storage identified by the given index ID.
+ *
+ * @param indexId Index ID which storage will be destroyed.
+ * @return Future that will be completed as soon as the storage has been
destroyed.
+ */
+ public CompletableFuture<Void> destroyIndex(int indexId) {
+ return busy(() -> {
+ throwExceptionIfStorageNotInRunnableState();
Review Comment:
In theory, rebalancing or clearing a partition should not interfere, but it
will be handled differently.
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/index/sorted/PageMemorySortedIndexStorage.java:
##########
@@ -226,15 +228,16 @@ public void updateDataStructures(IndexColumnsFreeList
freeList, SortedIndexTree
* @param executor {@link GradualTaskExecutor} on which to destroy.
* @throws StorageException If something goes wrong.
*/
- public void startDestructionOn(GradualTaskExecutor executor) throws
StorageException {
+ public CompletableFuture<Void> startDestructionOn(GradualTaskExecutor
executor) throws StorageException {
Review Comment:
There is no description of the return value.
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/util/MvPartitionStorages.java:
##########
@@ -432,6 +432,23 @@ private String
createStorageInProgressOfRebalanceErrorMessage(int partitionId) {
return null;
}
+ /**
+ * Returns a list of all existing storages.
+ */
+ public List<T> getAll() {
Review Comment:
It would be necessary to handle the situation when, for example, during
rebalancing, we understand that the node no longer owns the partition and must
delete it, and we are trying to delete the index.
And also think about restoring the destruction of the index to avoid races.
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/AbstractPageMemoryMvPartitionStorage.java:
##########
@@ -969,4 +977,44 @@ IndexMeta createIndexMetaForNewIndex(int indexId) {
return sortedIndexes.get(indexId);
});
}
+
+ /**
+ * Destroys an index storage identified by the given index ID.
+ *
+ * @param indexId Index ID which storage will be destroyed.
+ * @return Future that will be completed as soon as the storage has been
destroyed.
+ */
+ public CompletableFuture<Void> destroyIndex(int indexId) {
+ return busy(() -> {
+ throwExceptionIfStorageNotInRunnableState();
+
+ CompletableFuture<Void> result = nullCompletedFuture();
+
+ PageMemoryHashIndexStorage hashIndexStorage =
hashIndexes.remove(indexId);
+
+ if (hashIndexStorage != null) {
+ assert !sortedIndexes.containsKey(indexId);
Review Comment:
Are you sure a check is needed?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/AbstractPageMemoryMvPartitionStorage.java:
##########
@@ -969,4 +977,44 @@ IndexMeta createIndexMetaForNewIndex(int indexId) {
return sortedIndexes.get(indexId);
});
}
+
+ /**
+ * Destroys an index storage identified by the given index ID.
+ *
+ * @param indexId Index ID which storage will be destroyed.
+ * @return Future that will be completed as soon as the storage has been
destroyed.
+ */
+ public CompletableFuture<Void> destroyIndex(int indexId) {
+ return busy(() -> {
+ throwExceptionIfStorageNotInRunnableState();
+
+ CompletableFuture<Void> result = nullCompletedFuture();
+
+ PageMemoryHashIndexStorage hashIndexStorage =
hashIndexes.remove(indexId);
+
+ if (hashIndexStorage != null) {
+ assert !sortedIndexes.containsKey(indexId);
+
+ result =
hashIndexStorage.startDestructionOn(destructionExecutor)
+ .whenComplete((v, e) -> hashIndexStorage.close());
+ }
+
+ PageMemorySortedIndexStorage sortedIndexStorage =
sortedIndexes.remove(indexId);
+
+ if (sortedIndexStorage != null) {
+ result =
sortedIndexStorage.startDestructionOn(destructionExecutor)
+ .whenComplete((v, e) -> sortedIndexStorage.close());
+ }
+
+ return result.thenRun(() -> runConsistently(locker -> {
Review Comment:
Maybe thenRunAsync ?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/PersistentPageMemoryStorageEngine.java:
##########
@@ -77,6 +87,8 @@ public class PersistentPageMemoryStorageEngine implements
StorageEngine {
@Nullable
private volatile CheckpointManager checkpointManager;
+ private volatile ExecutorService destructionExecutor;
Review Comment:
Should we use some other pool instead of creating a new one? otherwise we
are trying to get rid of them little by little.
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/index/hash/PageMemoryHashIndexStorage.java:
##########
@@ -150,15 +152,16 @@ public void remove(IndexRow row) throws StorageException {
* @param executor {@link GradualTaskExecutor} on which to destroy.
* @throws StorageException If something goes wrong.
*/
- public void startDestructionOn(GradualTaskExecutor executor) throws
StorageException {
+ public CompletableFuture<Void> startDestructionOn(GradualTaskExecutor
executor) throws StorageException {
Review Comment:
There is no description of the return value.
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/AbstractPageMemoryTableStorage.java:
##########
@@ -180,7 +180,19 @@ public HashIndexStorage getOrCreateHashIndex(int
partitionId, StorageHashIndexDe
@Override
public CompletableFuture<Void> destroyIndex(int indexId) {
- throw new UnsupportedOperationException("Not implemented yet");
+ return busy(() -> {
+ List<AbstractPageMemoryMvPartitionStorage> storages =
mvPartitionStorages.getAll();
+
+ var destroyFutures = new CompletableFuture[storages.size()];
+
+ for (int i = 0; i < storages.size(); i++) {
+ AbstractPageMemoryMvPartitionStorage storage = storages.get(i);
+
+ destroyFutures[i] = storage.runConsistently(locker ->
storage.destroyIndex(indexId));
Review Comment:
I think that calling `storage.destroyIndex(indexId)` could bring us trouble
if any partition is in the process of cleaning or rebalancing.
--
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]
