rpuch commented on code in PR #4684:
URL: https://github.com/apache/ignite-3/pull/4684#discussion_r1833856630


##########
modules/replicator/src/main/java/org/apache/ignite/internal/replicator/ReplicaManager.java:
##########
@@ -1086,16 +1097,64 @@ private void idleSafeTimeSync() {
         }
     }
 
-    private void sendSafeTimeSyncIfReplicaReady(CompletableFuture<Replica> 
replicaFuture) {
-        if (isCompletedSuccessfully(replicaFuture)) {
-            Replica replica = replicaFuture.join();
+    private void sendSafeTimeSyncIfReplicaReady(CompletableFuture<Replica> 
replicaFuture, HybridTimestamp proposedSafeTime) {
+        if (!isCompletedSuccessfully(replicaFuture)) {
+            return;
+        }
 
-            ReplicaSafeTimeSyncRequest req = 
REPLICA_MESSAGES_FACTORY.replicaSafeTimeSyncRequest()
-                    .groupId(toReplicationGroupIdMessage(replica.groupId()))
-                    .build();
+        Replica replica = replicaFuture.join();
 
-            replica.processRequest(req, localNodeId);
+        if (!shouldAdvanceIdleSafeTime(replica, proposedSafeTime)) {
+            // If previous attempt might still be waiting on the Metastorage 
SafeTime, we should not send the command ourselves as this
+            // might be an indicator that Metastorage SafeTime has stuck for 
some time; if we send the command, it will have to add its
+            // future, increasing (most probably, uselessly) heap pressure.
+            return;
         }
+
+        getOrCreateContext(replica).lastIdleSafeTimeProposal = 
proposedSafeTime;
+
+        ReplicaSafeTimeSyncRequest req = 
REPLICA_MESSAGES_FACTORY.replicaSafeTimeSyncRequest()
+                .groupId(toReplicationGroupIdMessage(replica.groupId()))
+                .proposedSafeTime(proposedSafeTime)
+                .build();
+
+        replica.processRequest(req, localNodeId).whenComplete((res, ex) -> {
+            if (ex != null) {
+                LOG.error("Could not advance safe time for {} to {}", 
replica.groupId(), proposedSafeTime);
+            }
+        });
+    }
+
+    private boolean shouldAdvanceIdleSafeTime(Replica replica, HybridTimestamp 
proposedSafeTime) {
+        if (placementDriver.getCurrentPrimaryReplica(replica.groupId(), 
proposedSafeTime) != null) {

Review Comment:
   We call `clock.now()` just once for the whole batch; so for each individual 
replica, `clock.now()` is not called, and there is no such calls that can be 
saved



-- 
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-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to