[GitHub] [ignite-3] sashapolo commented on a diff in pull request #989: IGNITE-17077 persistedIndex implementation for PDS

[GitBox <gi...@apache.org>]

sashapolo commented on code in PR #989:
URL: https://github.com/apache/ignite-3/pull/989#discussion_r941221260


##########
modules/page-memory/src/main/java/org/apache/ignite/internal/pagememory/configuration/schema/PageMemoryCheckpointConfigurationSchema.java:
##########
@@ -36,6 +36,11 @@ public class PageMemoryCheckpointConfigurationSchema {
     @Value(hasDefault = true)
     public int frequencyDeviation = 40;
 
+    /** Delay before executing a checkpoint triggered by RAFT. */
+    @Range(min = 0)
+    @Value(hasDefault = true)
+    public int cpDelayMillis = 200;

Review Comment:
   I would prefer to name it `checkpointDelayMillis`



##########
modules/storage-api/src/test/java/org/apache/ignite/internal/storage/AbstractMvPartitionStorageTest.java:
##########
@@ -679,4 +682,25 @@ void scanWithTxIdThrowsWhenOtherTransactionHasUncommittedChanges() {
 
         assertThrows(TxIdMismatchException.class, cursor::next);
     }
+
+    @Test
+    void testAppliedIndex() {
+        storage.runConsistently(() -> {
+            assertEquals(0, storage.lastAppliedIndex());
+            assertEquals(0, storage.persistedIndex());
+
+            storage.lastAppliedIndex(1);
+
+            assertEquals(1, storage.lastAppliedIndex());
+            assertThat(storage.persistedIndex(), is(lessThanOrEqualTo(1L)));
+
+            return null;
+        });
+
+        CompletableFuture<Void> flushFuture = storage.flush();
+
+        assertThat(flushFuture, willBe(equalTo(null)));

Review Comment:
   there's a `willCompleteSuccessfully` matcher



##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/PersistentPageMemoryMvPartitionStorage.java:
##########
@@ -66,4 +109,53 @@ public <V> V runConsistently(WriteClosure<V> closure) throws StorageException {
             checkpointTimeoutLock.checkpointReadUnlock();
         }
     }
+
+    /** {@inheritDoc} */
+    @Override
+    public CompletableFuture<Void> flush() {
+        CheckpointProgress lastCheckpoint = checkpointManager.lastCheckpointProgress();
+
+        CheckpointProgress scheduledCheckpoint;
+
+        if (lastCheckpoint != null && meta.metaSnapshot(lastCheckpoint.id()).lastAppliedIndex() >= meta.lastAppliedIndex()) {
+            scheduledCheckpoint = lastCheckpoint;
+        } else {
+            PersistentPageMemoryStorageEngineView engineCfg = tableStorage.engine().configuration().value();
+
+            int cpDelayMillis = engineCfg.checkpoint().cpDelayMillis();
+            scheduledCheckpoint = checkpointManager.scheduleCheckpoint(cpDelayMillis, "Triggered by replicator.");

Review Comment:
   ```suggestion
               scheduledCheckpoint = checkpointManager.scheduleCheckpoint(cpDelayMillis, "Triggered by replicator");
   ```



##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/mv/PersistentPageMemoryMvPartitionStorage.java:
##########
@@ -66,4 +109,53 @@ public <V> V runConsistently(WriteClosure<V> closure) throws StorageException {
             checkpointTimeoutLock.checkpointReadUnlock();
         }
     }
+
+    /** {@inheritDoc} */
+    @Override
+    public CompletableFuture<Void> flush() {
+        CheckpointProgress lastCheckpoint = checkpointManager.lastCheckpointProgress();
+
+        CheckpointProgress scheduledCheckpoint;
+
+        if (lastCheckpoint != null && meta.metaSnapshot(lastCheckpoint.id()).lastAppliedIndex() >= meta.lastAppliedIndex()) {

Review Comment:
   how can lastAppliedIndex of a snapshot be larger then the current value?



##########
modules/storage-api/src/test/java/org/apache/ignite/internal/storage/AbstractMvPartitionStorageTest.java:
##########
@@ -679,4 +682,25 @@ void scanWithTxIdThrowsWhenOtherTransactionHasUncommittedChanges() {
 
         assertThrows(TxIdMismatchException.class, cursor::next);
     }
+
+    @Test
+    void testAppliedIndex() {

Review Comment:
   Please leave a comment about what this test actually tests and what invariants it checks



##########
modules/storage-page-memory/src/test/java/org/apache/ignite/internal/storage/pagememory/mv/PersistentPageMemoryMvPartitionStorageTest.java:
##########
@@ -73,6 +73,8 @@ void setUp() throws Exception {
 
         longJvmPauseDetector.start();
 
+        engineConfig.checkpoint().cpDelayMillis().update(0).get(1, TimeUnit.SECONDS);

Review Comment:
   can it be specified as a string in `InjectConfiguration`?



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