[GitHub] [ignite-3] sashapolo commented on a diff in pull request #2506: IGNITE-20290 Command reordering wrt safe time in MetaStorage

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

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


##########
modules/metastorage/src/integrationTest/java/org/apache/ignite/internal/metastorage/impl/ItMetaStorageMultipleNodesAbstractTest.java:
##########
@@ -585,6 +588,66 @@ void testIdleSafeTimePropagationLeaderTransferred(TestInfo testInfo) throws Exce
         assertThat(secondNodeTime.waitFor(now), willCompleteSuccessfully());
     }
 
+    /**
+     * Tests that idle safe time propagation does not advance safe time while watches of a normal command are being executed.
+     */
+    @Test
+    void testIdleSafeTimePropagationAndNormalSafeTimePropagationInteraction(TestInfo testInfo) throws Exception {
+        // Enable idle safe time sync.
+        CompletableFuture<Void> updateIdleSyncTimeIntervalFuture = metaStorageConfiguration.idleSyncTimeInterval().update(100L);
+        assertThat(updateIdleSyncTimeIntervalFuture, willCompleteSuccessfully());
+
+        Node firstNode = startNode(testInfo);

Review Comment:
   Do we really need a multi-node test here? I can imagine a "unit" test where we start a MetaStorageManager, block its watches and then manually run a time sync command. 



##########
modules/metastorage/src/integrationTest/java/org/apache/ignite/internal/metastorage/impl/ItMetaStorageMultipleNodesAbstractTest.java:
##########
@@ -585,6 +588,66 @@ void testIdleSafeTimePropagationLeaderTransferred(TestInfo testInfo) throws Exce
         assertThat(secondNodeTime.waitFor(now), willCompleteSuccessfully());
     }
 
+    /**
+     * Tests that idle safe time propagation does not advance safe time while watches of a normal command are being executed.
+     */
+    @Test
+    void testIdleSafeTimePropagationAndNormalSafeTimePropagationInteraction(TestInfo testInfo) throws Exception {
+        // Enable idle safe time sync.
+        CompletableFuture<Void> updateIdleSyncTimeIntervalFuture = metaStorageConfiguration.idleSyncTimeInterval().update(100L);
+        assertThat(updateIdleSyncTimeIntervalFuture, willCompleteSuccessfully());
+
+        Node firstNode = startNode(testInfo);
+        Node secondNode = startNode(testInfo);
+
+        firstNode.cmgManager.initCluster(List.of(firstNode.name()), List.of(firstNode.name()), "test");
+
+        assertThat(firstNode.cmgManager.onJoinReady(), willCompleteSuccessfully());
+        assertThat(secondNode.cmgManager.onJoinReady(), willCompleteSuccessfully());
+
+        var key = new ByteArray("foo");
+        byte[] value = "bar".getBytes(StandardCharsets.UTF_8);
+
+        AtomicBoolean watchCompleted = new AtomicBoolean(false);
+        CompletableFuture<HybridTimestamp> watchEventTsFuture = new CompletableFuture<>();
+
+        secondNode.metaStorageManager.registerExactWatch(key, new WatchListener() {
+            @Override
+            public CompletableFuture<Void> onUpdate(WatchEvent event) {
+                watchEventTsFuture.complete(event.timestamp());
+
+                // The future will set the flag and complete after 300ms to allow idle safe time mechanism (which ticks each 100ms)
+                // to advance SafeTime (if there is still a bug for which this test is written).
+                return new CompletableFuture<Void>()
+                        .orTimeout(300, TimeUnit.MILLISECONDS)
+                        .exceptionally(ex -> {
+                            assert ex instanceof TimeoutException;

Review Comment:
   This assertion will not work, because you ignore all exceptions below



##########
modules/metastorage/src/main/java/org/apache/ignite/internal/metastorage/server/persistence/RocksDbKeyValueStorage.java:
##########
@@ -1664,4 +1664,17 @@ public void unregisterRevisionUpdateListener(RevisionUpdateListener listener) {
     public CompletableFuture<Void> notifyRevisionUpdateListenerOnStart(long newRevision) {
         return watchProcessor.notifyUpdateRevisionListeners(newRevision);
     }
+
+    @Override
+    public void advanceSafeTime(HybridTimestamp newSafeTime) {
+        rwLock.writeLock().lock();

Review Comment:
   Why do we need to take the lock here?



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