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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
