You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/11/01 16:11:57 UTC
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #1286: IGNITE-18021 Creating an API for full rebalance without losing user data in MvPartitionStorage on receiver
sashapolo commented on code in PR #1286:
URL: https://github.com/apache/ignite-3/pull/1286#discussion_r1010564838
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/engine/MvTableStorage.java:
##########
@@ -162,4 +163,32 @@ default IndexStorage getOrCreateIndex(int partitionId, UUID indexId) {
* @throws StorageException If an error has occurred during the destruction of the storage.
*/
void destroy() throws StorageException;
+
+ /**
+ * Prepares the partition storage for rebalancing: makes a backup and creates a new one.
Review Comment:
What do you mean by "a new one"? A new storage? Please write this explicitly
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/AbstractMvTableStorageTest.java:
##########
@@ -77,6 +88,8 @@ public abstract class AbstractMvTableStorageTest extends BaseMvStoragesTest {
private TableIndexView hashIdx;
+ private HybridClock clock = new HybridClockImpl();
Review Comment:
`final`?
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/engine/MvTableStorage.java:
##########
@@ -162,4 +163,32 @@ default IndexStorage getOrCreateIndex(int partitionId, UUID indexId) {
* @throws StorageException If an error has occurred during the destruction of the storage.
*/
void destroy() throws StorageException;
+
+ /**
+ * Prepares the partition storage for rebalancing: makes a backup and creates a new one.
+ *
+ * @param partitionId Partition ID.
+ * @param executor Executor.
+ * @return Future, if completed without errors, then {@link #getMvPartition} will return a new (empty) partition storage.
+ */
+ CompletableFuture<Void> startRebalanceMvPartition(int partitionId, Executor executor);
+
+ /**
+ * Aborts rebalancing of the partition storage if it was started: restores the backup and deletes the new one.
+ *
+ * @param partitionId Partition ID.
+ * @param executor Executor.
+ * @return Future, upon completion of which (even if with an error) {@link #getMvPartition} will return the partition storage restored
+ * from the backup.
+ */
+ CompletableFuture<Void> abortRebalanceMvPartition(int partitionId, Executor executor);
+
+ /**
+ * Finishes a successful partition storage rebalance if it has been started: deletes the backup and saves the new one.
Review Comment:
Same about the "new one"
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/engine/MvTableStorage.java:
##########
@@ -162,4 +163,32 @@ default IndexStorage getOrCreateIndex(int partitionId, UUID indexId) {
* @throws StorageException If an error has occurred during the destruction of the storage.
*/
void destroy() throws StorageException;
+
+ /**
+ * Prepares the partition storage for rebalancing: makes a backup and creates a new one.
+ *
+ * @param partitionId Partition ID.
+ * @param executor Executor.
+ * @return Future, if completed without errors, then {@link #getMvPartition} will return a new (empty) partition storage.
+ */
+ CompletableFuture<Void> startRebalanceMvPartition(int partitionId, Executor executor);
+
+ /**
+ * Aborts rebalancing of the partition storage if it was started: restores the backup and deletes the new one.
Review Comment:
Same about the "new one"
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/impl/TestMvTableStorage.java:
##########
@@ -101,12 +106,10 @@ public MvPartitionStorage getMvPartition(int partitionId) {
@Override
public void destroyPartition(int partitionId) throws StorageException {
- Integer boxedPartitionId = partitionId;
Review Comment:
Why did you change this code?
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/engine/MvTableStorage.java:
##########
@@ -162,4 +163,32 @@ default IndexStorage getOrCreateIndex(int partitionId, UUID indexId) {
* @throws StorageException If an error has occurred during the destruction of the storage.
*/
void destroy() throws StorageException;
+
+ /**
+ * Prepares the partition storage for rebalancing: makes a backup and creates a new one.
+ *
+ * @param partitionId Partition ID.
+ * @param executor Executor.
+ * @return Future, if completed without errors, then {@link #getMvPartition} will return a new (empty) partition storage.
+ */
+ CompletableFuture<Void> startRebalanceMvPartition(int partitionId, Executor executor);
+
+ /**
+ * Aborts rebalancing of the partition storage if it was started: restores the backup and deletes the new one.
Review Comment:
You should describe what will happen if this method is called without calling `startRebalanceMvPartition`
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/engine/MvTableStorage.java:
##########
@@ -162,4 +163,32 @@ default IndexStorage getOrCreateIndex(int partitionId, UUID indexId) {
* @throws StorageException If an error has occurred during the destruction of the storage.
*/
void destroy() throws StorageException;
+
+ /**
+ * Prepares the partition storage for rebalancing: makes a backup and creates a new one.
Review Comment:
You should probable add a description about the contract of these methods, i.e. how are they intended to be used.
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/impl/TestMvTableStorage.java:
##########
@@ -176,4 +179,35 @@ public void stop() throws StorageException {
@Override
public void destroy() throws StorageException {
}
+
+ @Override
+ public CompletableFuture<Void> startRebalanceMvPartition(int partitionId, Executor executor) {
+ MvPartitionStorage oldPartitionStorage = partitions.get(partitionId);
+
+ assert oldPartitionStorage != null : "Partition does not exists: " + partitionId;
Review Comment:
```suggestion
assert oldPartitionStorage != null : "Partition does not exist: " + partitionId;
```
##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/AbstractMvTableStorageTest.java:
##########
@@ -299,6 +312,92 @@ public void testMisconfiguredIndices() {
);
}
+ @Test
+ protected void testStartRebalanceMvPartition() throws Exception {
Review Comment:
why `protected`? All other tests are `public`
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/engine/MvTableStorage.java:
##########
@@ -162,4 +163,32 @@ default IndexStorage getOrCreateIndex(int partitionId, UUID indexId) {
* @throws StorageException If an error has occurred during the destruction of the storage.
*/
void destroy() throws StorageException;
+
+ /**
+ * Prepares the partition storage for rebalancing: makes a backup and creates a new one.
+ *
+ * @param partitionId Partition ID.
+ * @param executor Executor.
Review Comment:
You should specify, why is this executor needed, which operations will be executed on it
##########
modules/storage-api/src/main/java/org/apache/ignite/internal/storage/engine/MvTableStorage.java:
##########
@@ -162,4 +163,32 @@ default IndexStorage getOrCreateIndex(int partitionId, UUID indexId) {
* @throws StorageException If an error has occurred during the destruction of the storage.
*/
void destroy() throws StorageException;
+
+ /**
+ * Prepares the partition storage for rebalancing: makes a backup and creates a new one.
+ *
+ * @param partitionId Partition ID.
+ * @param executor Executor.
+ * @return Future, if completed without errors, then {@link #getMvPartition} will return a new (empty) partition storage.
+ */
+ CompletableFuture<Void> startRebalanceMvPartition(int partitionId, Executor executor);
+
+ /**
+ * Aborts rebalancing of the partition storage if it was started: restores the backup and deletes the new one.
+ *
+ * @param partitionId Partition ID.
+ * @param executor Executor.
+ * @return Future, upon completion of which (even if with an error) {@link #getMvPartition} will return the partition storage restored
+ * from the backup.
+ */
+ CompletableFuture<Void> abortRebalanceMvPartition(int partitionId, Executor executor);
+
+ /**
+ * Finishes a successful partition storage rebalance if it has been started: deletes the backup and saves the new one.
Review Comment:
You should describe what will happen if this method is called without calling `startRebalanceMvPartition`
--
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