New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

IGNITE-22172 Make ignite-collocation-feature branch stable #102

Merged

vldpyatkov merged 15 commits into ignite-collocation-feature from ignite-22172-in-collocation-feature

May 15, 2024

vldpyatkov commented May 10, 2024

No description provided.

vldpyatkov added 5 commits

May 10, 2024 13:02


          IGNITE-22172 Make ignite-collocation-feature branch stable

eefc0c2


          Fixed handling of replica unavailable exception.

ff0e04c


          Add detail to the primary replica exception.

c2fdcae

WIP

c663cb2


          Added an additional message to wait for the primary replica to be rea…

5a5330a

…dy before returning the primary replica for the placement driver API.

vldpyatkov force-pushed the ignite-22172-in-collocation-feature branch from e227c44 to 5a5330a Compare

May 13, 2024 17:18

vldpyatkov added 6 commits

May 13, 2024 20:42


          Optimized of MC threads using.

91add97


          Optimization MC load.

3e65f95

WIP

12299e6

WIP

51decd9

WIP

927512e


          Fixed test naming.

alievmirza self-requested a review

May 14, 2024 13:38

alievmirza reviewed

View reviewed changes

modules/core/src/main/java/org/apache/ignite/internal/replicator/ZonePartitionId.java Outdated Show resolved Hide resolved

.../placement-driver/src/main/java/org/apache/ignite/internal/placementdriver/LeaseUpdater.java

    
                                  return;

                              }

                              if (!success) {

                                  LOG.debug("Lease update invocation failed");

                                  LOG.warn("Lease update invocation failed.");

alievmirza May 14, 2024

do we need warn here?

Author

vldpyatkov May 14, 2024

I think a warning is better than just hiding this issue. In ordinary cases, the PD should process all the updates.
Currently, this message might appear because we have a concurrent update of the MC key.

...ent-driver/src/main/java/org/apache/ignite/internal/placementdriver/leases/LeaseTracker.java Outdated Show resolved Hide resolved

modules/replicator/src/main/java/org/apache/ignite/internal/replicator/Replica.java

    
            @@ -100,7 +104,7 @@ public class Replica {
          
                  private final ClockService clockService;

                  private final CompletableFuture<Void> waitForActualStateFuture = new CompletableFuture<>();

                  private final AtomicReference<CompletableFuture<Void>> waitForActualStateFuture = new AtomicReference<>();

alievmirza May 14, 2024

Why do we need this change?

Author

vldpyatkov May 14, 2024

It is a better way to avoid starting to wait for the actual state process when several messages are sent.
When one message is in process, the other will wait for the future.

modules/replicator/src/main/java/org/apache/ignite/internal/replicator/Replica.java

                   }
                   private CompletableFuture<Void> sendPrimaryReplicaChangeToReplicationGroup(long leaseStartTime) {
                       PrimaryReplicaChangeCommand cmd = REPLICA_MESSAGES_FACTORY.primaryReplicaChangeCommand()
                               .leaseStartTime(leaseStartTime)
                               .build();
-                      return raftClient.run(cmd);
+                      return raftClient.run(cmd).thenRun(() -> {
+                          if (!waitForActualStateFuture.compareAndSet(null, nullCompletedFuture())) {

alievmirza May 14, 2024

Why do we need this here?

Author

vldpyatkov May 14, 2024

The methodoid is the final phase of the wating for the actual state process.

modules/replicator/src/main/java/org/apache/ignite/internal/replicator/Replica.java Outdated

                       return retryOperationUntilSuccess(raftClient::readIndex, e -> currentTimeMillis() > expirationTime, executor)
                               .orTimeout(timeout, TimeUnit.MILLISECONDS)
-                              .thenCompose(storageIndexTracker::waitFor)
-                              .thenRun(() -> waitForActualStateFuture.complete(null));
+                              .thenCompose(idx -> storageIndexTracker.waitFor(idx));

alievmirza May 14, 2024

why have you moved completion of the future from here?

Author

vldpyatkov May 14, 2024 •

edited

Currently, the process of waiting for the actual state contains two parts:

waitForActualState
sendPrimaryReplicaChangeToReplicationGroup

The process is completed when the last stage is completed.
If we look at all places where the waitForActualState is used, you will see the sendPrimaryReplicaChangeToReplicationGroup is always used after.

modules/replicator/src/main/java/org/apache/ignite/internal/replicator/Replica.java Outdated Show resolved Hide resolved

...replicator/src/main/java/org/apache/ignite/internal/replicator/ReplicaAwareLeaseTracker.java Outdated Show resolved Hide resolved

modules/replicator/src/main/java/org/apache/ignite/internal/replicator/Replica.java Outdated

+                                      return processWaitReplicaStateMessage(waitReplicaStateMsg)
+                                              .thenApply(unused -> EMPTY_REPLICA_RESULT);
+                                  } else {
+                                      waitForActualStateFuture.get().thenApply(unused -> EMPTY_REPLICA_RESULT);

alievmirza May 14, 2024

What does this line of code do?

Author

vldpyatkov May 14, 2024

This code sends the empty result as a response to WaitReplicaStateMessage. This branch happens if another WaitReplicaStateMessage has already started.

...tionTest/java/org/apache/ignite/internal/disaster/ItDisasterRecoveryReconfigurationTest.java

                       assertTrue(IgniteTestUtils.waitForCondition(() -> {
                           long causalityToken = node.metaStorageManager().appliedRevision();
-                          long msSafeTime = node.metaStorageManager().timestampByRevision(causalityToken).longValue();
-                          int catalogVersion = node.catalogManager().activeCatalogVersion(msSafeTime);
+                          int catalogVersion = node.catalogManager().latestCatalogVersion();

alievmirza May 14, 2024 •

edited

Why have you made this change?

Author

vldpyatkov May 14, 2024

It was an incorrect test because it capturedd not final catalog state.

ibessonov May 15, 2024

Agreed, ms idle safe time is not taken into account while retrieving msSafeTime, my code was buggy

vldpyatkov added 4 commits

May 14, 2024 18:39


          Fixed after review for M.Aliev


          Small but meaningful fix.

40f04d6


          Added missed return result from the replica side.

8d53fbf


          Fixed concurrent update issue when the LeaseGrantMessage is processing.

alievmirza approved these changes

View reviewed changes

vldpyatkov merged commit a0fa5e8 into ignite-collocation-feature

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment