Re: [PR] IGNITE-22638 Add the ability to subscribe to the next flush without triggering it [ignite-3]

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

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


##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/engine/AbstractStorageEngineTest.java:
##########
@@ -288,6 +291,54 @@ void testIndexRecoveryForMultipleTables(@Mock StorageIndexDescriptorSupplier ind
         }
     }
 
+    /**
+     * Ensures that {@code flush(false)} does not trigger a flush explicitly, but only subscribes to the next scheduled one.
+     */
+    @Test
+    void testSubscribeToFlush() throws Exception {
+        assumeFalse(storageEngine.isVolatile());
+
+        int tableId = 1;
+        int lastAppliedIndex = 10;
+        int lastAppliedTerm = 20;
+
+        StorageTableDescriptor tableDescriptor = new StorageTableDescriptor(tableId, 1, DEFAULT_STORAGE_PROFILE);
+        StorageIndexDescriptorSupplier indexSupplier = mock(StorageIndexDescriptorSupplier.class);
+
+        MvTableStorage mvTableStorage = storageEngine.createMvTable(tableDescriptor, indexSupplier);
+
+        try (AutoCloseable ignored0 = mvTableStorage::close) {
+            CompletableFuture<MvPartitionStorage> mvPartitionStorageFuture = mvTableStorage.createMvPartition(0);
+
+            assertThat(mvPartitionStorageFuture, willCompleteSuccessfully());
+            MvPartitionStorage mvPartitionStorage = mvPartitionStorageFuture.join();
+
+            try (AutoCloseable ignored1 = mvPartitionStorage::close) {
+                assertThat(mvPartitionStorage.flush(), willCompleteSuccessfully());
+
+                {
+                    mvPartitionStorage.runConsistently(locker -> {
+                        mvPartitionStorage.lastApplied(lastAppliedIndex, lastAppliedTerm);
+
+                        return null;
+                    });
+
+                    CompletableFuture<Void> subscribeFuture1 = mvPartitionStorage.flush(false);
+                    assertThat(subscribeFuture1, willTimeoutFast());
+
+                    CompletableFuture<Void> subscribeFuture2 = mvPartitionStorage.flush(false);
+                    assertThat(subscribeFuture2, willTimeoutFast());
+
+                    assertSame(subscribeFuture1, subscribeFuture2);
+                    assertThat(mvPartitionStorage.flush(), willCompleteSuccessfully());
+
+                    assertTrue(subscribeFuture1.isDone());
+                    assertTrue(subscribeFuture2.isDone());

Review Comment:
   Please make sure that there's no data race here. Does this test fail if you run it 1000 times?
   What I mean: here you claim that `flush(false)` futures must be completed before `flush()` future is completed. Do we really guarantee that?



##########
modules/storage-api/src/testFixtures/java/org/apache/ignite/internal/storage/engine/AbstractStorageEngineTest.java:
##########
@@ -288,6 +291,54 @@ void testIndexRecoveryForMultipleTables(@Mock StorageIndexDescriptorSupplier ind
         }
     }
 
+    /**
+     * Ensures that {@code flush(false)} does not trigger a flush explicitly, but only subscribes to the next scheduled one.
+     */
+    @Test
+    void testSubscribeToFlush() throws Exception {
+        assumeFalse(storageEngine.isVolatile());
+
+        int tableId = 1;
+        int lastAppliedIndex = 10;
+        int lastAppliedTerm = 20;
+
+        StorageTableDescriptor tableDescriptor = new StorageTableDescriptor(tableId, 1, DEFAULT_STORAGE_PROFILE);
+        StorageIndexDescriptorSupplier indexSupplier = mock(StorageIndexDescriptorSupplier.class);
+
+        MvTableStorage mvTableStorage = storageEngine.createMvTable(tableDescriptor, indexSupplier);
+
+        try (AutoCloseable ignored0 = mvTableStorage::close) {
+            CompletableFuture<MvPartitionStorage> mvPartitionStorageFuture = mvTableStorage.createMvPartition(0);
+
+            assertThat(mvPartitionStorageFuture, willCompleteSuccessfully());
+            MvPartitionStorage mvPartitionStorage = mvPartitionStorageFuture.join();
+
+            try (AutoCloseable ignored1 = mvPartitionStorage::close) {
+                assertThat(mvPartitionStorage.flush(), willCompleteSuccessfully());
+
+                {

Review Comment:
   This scope seems strange, what does it do?



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