ibessonov commented on code in PR #1325:
URL: https://github.com/apache/ignite-3/pull/1325#discussion_r1030257606
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/VolatilePageMemoryTableStorage.java:
##########
@@ -157,4 +155,10 @@ public CompletableFuture<Void>
finishRebalanceMvPartition(int partitionId) {
// TODO: IGNITE-18028 Implement
throw new UnsupportedOperationException();
}
+
+ @Override
+ public void destroyMvPartitionStorage(AbstractPageMemoryMvPartitionStorage
mvPartitionStorage) throws StorageException {
Review Comment:
Shouldn't this method be protected instead?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/index/sorted/PageMemorySortedIndexStorage.java:
##########
@@ -120,22 +188,63 @@ public void remove(IndexRow row) {
remove.afterCompletion();
} catch (IgniteInternalCheckedException e) {
throw new StorageException("Failed to remove value from index", e);
+ } finally {
+ closeBusyLock.leaveBusy();
}
}
@Override
public Cursor<IndexRow> scan(@Nullable BinaryTuplePrefix lowerBound,
@Nullable BinaryTuplePrefix upperBound, int flags) {
- boolean includeLower = (flags & GREATER_OR_EQUAL) != 0;
- boolean includeUpper = (flags & LESS_OR_EQUAL) != 0;
-
- SortedIndexRowKey lower = createBound(lowerBound, !includeLower);
-
- SortedIndexRowKey upper = createBound(upperBound, includeUpper);
+ if (!closeBusyLock.enterBusy()) {
+ throwStorageClosedException();
+ }
try {
- return map(sortedIndexTree.find(lower, upper),
this::toIndexRowImpl);
+ boolean includeLower = (flags & GREATER_OR_EQUAL) != 0;
+ boolean includeUpper = (flags & LESS_OR_EQUAL) != 0;
+
+ SortedIndexRowKey lower = createBound(lowerBound, !includeLower);
+
+ SortedIndexRowKey upper = createBound(upperBound, includeUpper);
+
+ Cursor<SortedIndexRow> cursor = sortedIndexTree.find(lower, upper);
+
+ return new Cursor<>() {
Review Comment:
Maybe we can extract a class to be reused
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/PersistentPageMemoryTableStorage.java:
##########
@@ -418,4 +430,43 @@ public CompletableFuture<Void>
finishRebalanceMvPartition(int partitionId) {
// TODO: IGNITE-18029 Implement
throw new UnsupportedOperationException();
}
+
+ @Override
+ public void destroyMvPartitionStorage(AbstractPageMemoryMvPartitionStorage
mvPartitionStorage) throws StorageException {
+ int partitionId = mvPartitionStorage.partitionId();
+
+ CompletableFuture<Void> previousFuture =
destroyFutureByPartitionId.put(partitionId, new CompletableFuture<>());
+
+ assert previousFuture == null : "Previous destruction of the partition
has not completed: " + partitionId;
Review Comment:
What's up with the future anyway? No one uses it, what's the point?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/AbstractPageMemoryMvPartitionStorage.java:
##########
@@ -834,23 +958,45 @@ public boolean hasNext() {
}
while (true) {
- if (!treeCursor.hasNext()) {
- iterationExhausted = true;
- return false;
+ if (!closeBusyLock.enterBusy()) {
+ throwStorageClosedException();
}
- VersionChain chain = treeCursor.next();
- ReadResult result = findLatestRowVersion(chain);
+ try {
+ if (!treeCursor.hasNext()) {
+ iterationExhausted = true;
+ return false;
+ }
- if (result.isEmpty() && !result.isWriteIntent()) {
- continue;
- }
+ VersionChain chain = treeCursor.next();
+ ReadResult result = findLatestRowVersion(chain);
- nextRead = result;
- currentChain = chain;
+ if (result.isEmpty() && !result.isWriteIntent()) {
+ continue;
+ }
- return true;
+ nextRead = result;
+ currentChain = chain;
+
+ return true;
+ } finally {
+ closeBusyLock.leaveBusy();
+ }
}
}
}
+
+ /**
+ * Throws an exception that the storage is already closed.
+ */
+ protected void throwStorageClosedException() {
+ throw new StorageClosedException("Storage is already closed");
+ }
+
+ /**
+ * Returns the partition ID.
+ */
+ public int partitionId() {
Review Comment:
Please move this method a bit higher, ideally before any data access methods
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/index/hash/PageMemoryHashIndexStorage.java:
##########
@@ -123,11 +188,34 @@ public void remove(IndexRow row) throws StorageException {
remove.afterCompletion();
} catch (IgniteInternalCheckedException e) {
throw new StorageException("Failed to remove value from index", e);
+ } finally {
+ closeBusyLock.leaveBusy();
}
}
@Override
public void destroy() throws StorageException {
- //TODO IGNITE-17626 Implement.
+ // TODO: IGNITE-17626 Remove it
+ throw new UnsupportedOperationException();
+ }
+
+ /**
+ * Closes the hash index storage.
+ */
+ public void close() {
+ if (!STARTED.compareAndSet(this, true, false)) {
Review Comment:
Usually "not started" means "not YET started". Why don't you use same names
as everyone else? Why does it have to be different?
Uniformity in code is very important, it's always easier when you see a
common pattern.
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/PersistentPageMemoryTableStorage.java:
##########
@@ -418,4 +430,43 @@ public CompletableFuture<Void>
finishRebalanceMvPartition(int partitionId) {
// TODO: IGNITE-18029 Implement
throw new UnsupportedOperationException();
}
+
+ @Override
+ public void destroyMvPartitionStorage(AbstractPageMemoryMvPartitionStorage
mvPartitionStorage) throws StorageException {
+ int partitionId = mvPartitionStorage.partitionId();
+
+ CompletableFuture<Void> previousFuture =
destroyFutureByPartitionId.put(partitionId, new CompletableFuture<>());
+
+ assert previousFuture == null : "Previous destruction of the partition
has not completed: " + partitionId;
Review Comment:
Why not just return the old future?
--
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]
