rpuch commented on code in PR #4684:
URL: https://github.com/apache/ignite-3/pull/4684#discussion_r1833790879
##########
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) {
+ // We know a primary is guaranteed to be valid at the proposed
safe time, so, when processing the SafeTime propagation request,
+ // we'll not need to wait for Metastorage SafeTime, so processing
will be cheap.
+ return true;
Review Comment:
1. If we await for the success of the previous attempt we might stuck
forever if it never completes its future (for whatever reason). This was the
main reason you rejected previous attempt to fix this problem in #4111
2. The idea of this fix is to evade accumulating futures waiting on
Metastorage SafeTime tracker. If `getCurrentPrimaryReplica()` returns a
non-null value, we are sure there will no be wait even for THIS attempt, so we
will not create a future. Other aspects don't bother us here (as they don't
break correctness)
--
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
