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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to