[FileCopy Protocol] Bootstrap Controller #2974

DevenAhluwalia · 2024-12-19T15:31:42Z

This PR implements this design doc :- https://docs.google.com/document/d/1u57C0BU8oMMuMHC6Fh_B-6R612EaQb2AxtnBDlN6sAs/edit?tab=t.0

Change log :-

Base init for BootstrapController
BootstrapController registered as a StatePartitionlistner
Test cases for BootstrapController
Some utils like isAnyLogSegmentExists

(cherry picked from commit 1ce57b3)

(cherry picked from commit e991679)

ambry-api/src/main/java/com/github/ambry/config/StoreConfig.java

ambry-store/src/main/java/com/github/ambry/store/BootstrapController.java

ambry-api/src/main/java/com/github/ambry/server/StoreManager.java

ambry-clustermap/src/main/java/com/github/ambry/clustermap/HelixParticipant.java

ambry-store/src/main/java/com/github/ambry/store/BootstrapController.java

aga9900 · 2024-12-20T08:25:14Z

ambry-store/src/main/java/com/github/ambry/store/BootstrapController.java

+      listenerToInvoke.onPartitionBecomeBootstrapFromOffline(partitionName);
+      if (listenerToInvoke == fileCopyManagerListener) {
+        try {
+          storeManager.getPrimaryClusterParticipant().getReplicaSyncUpManager().waitForFileCopyCompleted(partitionName);


Can we pass replica synup manager object to this class directly?

replicaSyncUpManager is a prop of HelixParticipant class.
And we need to invoke replicaSyncUpManager. waitForFileCopyCompleted() for the storeManager's ClusterParticipant.
How can we pass replicaSyncUpManager object to BootstrapController class?

Update; as part of another comment, Iv currently removed getPrimaryClusterParticipant from StoreManager interface. I am now using the cluster-participant sent from AmbryServer and I am using cluster-participant.getReplicaSyncUpManager() in the latest diff.

ambry-store/src/main/java/com/github/ambry/store/BootstrapController.java

aga9900 · 2024-12-20T08:30:36Z

ambry-store/src/main/java/com/github/ambry/store/BootstrapController.java

+      // An optimisation could be explored to only delete incomplete datasets
+      // More about this can be found in this comment :-
+      // https://docs.google.com/document/d/1u57C0BU8oMMuMHC6Fh_B-6R612EaQb2AxtnBDlN6sAs/edit?disco=AAABZyj2rHo
+      storeManager.removeBlobStore(partitionId);


We cannot delete the directory in this case. This will break complete flow.

This is FC.Incomplete -> R case as we had discussed in this doc :- https://docs.google.com/document/d/1u57C0BU8oMMuMHC6Fh_B-6R612EaQb2AxtnBDlN6sAs/edit?tab=t.0#heading=h.seiobqi3qdu3

If we don't remove the BlobStore, should we then instead explicitly delete FC_files?

Discussed with @aga9900
We will need to evaluate the impact of removeBlobStore call here.
Added a TODO.

ambry-cloud/src/main/java/com/github/ambry/cloud/CloudStorageManager.java

aga9900

Did a first pass of review.

piyujai · 2024-12-20T09:14:49Z

ambry-store/src/main/java/com/github/ambry/store/BootstrapController.java

+        StateModelListenerType.BootstrapControllerListener, new BootstrapControllerImpl());
+
+    Map<StateModelListenerType, PartitionStateChangeListener> partitionStateChangeListeners =
+        storeManager.getPrimaryClusterParticipant().getPartitionStateChangeListeners();


We should add null handling for storeManager.getPrimaryClusterParticipant()

As part of another comment, Iv currently removed getPrimaryClusterParticipant from StoreManager interface. I am now using the cluster-participant sent from AmbryServer.

Have added another generic-exception catch block anyway and we now throw StateTransitionException with type BootstrapControllerFailure.

ambry-store/src/main/java/com/github/ambry/store/BootstrapController.java

piyujai · 2024-12-20T09:16:30Z

ambry-store/src/main/java/com/github/ambry/store/BootstrapController.java

+       * https://docs.google.com/document/d/1u57C0BU8oMMuMHC6Fh_B-6R612EaQb2AxtnBDlN6sAs
+       * The decision branches can be better understood with the doc.
+       */
+      if (null == replica) {


nit: replica==null

Intentional :)
https://en.wikipedia.org/wiki/Yoda_conditions

ambry-store/src/main/java/com/github/ambry/store/BootstrapController.java

piyujai · 2024-12-20T09:17:57Z

ambry-store/src/main/java/com/github/ambry/store/BootstrapController.java

+      storeManager.removeBlobStore(partitionId);
+    }
+  }
+}


nit: newline

redundant newline at the EoF?

piyujai · 2024-12-20T09:20:22Z

ambry-utils/src/main/java/com/github/ambry/utils/Utils.java

+      }
+
+      @Override
+      public FileVisitResult postVisitDirectory(Path dir, IOException exc) {


Do we need this method?

ambry-store/src/main/java/com/github/ambry/store/StorageManager.java

aga9900 · 2024-12-20T10:05:25Z

ambry-store/src/main/java/com/github/ambry/store/StorageManager.java

@@ -449,6 +450,22 @@ public boolean controlCompactionForBlobStore(PartitionId id, boolean enabled) {
    return diskManager != null && diskManager.controlCompactionForBlobStore(id, enabled);
  }

+  @Override
+  public ClusterParticipant getPrimaryClusterParticipant() {


Can we pass cluster participant object to Bootstrap Controller.

Good point.
While AmbryServer is passing cluster-participant while initiating BootstrapController, it passed clusterParticipants to StorageManager and a lot of other classes.
Then, StorageManager extracts primaryClusterParticipant exactly the same way as AmbryServer does.

Looks like, it should be fine to just use the primaryClusterParticipant from AmbryServer directly.

Change incorporated.

DevenAhluwalia · 2024-12-20T15:38:18Z

ambry-clustermap/src/test/java/com/github/ambry/clustermap/TestReplica.java

+
+
+// Permits Replica to be constructed with a null Partition
+public class TestReplica extends Replica {


[Info] Needed this to be able to mock this from BootstrapController tests.

DevenAhluwalia · 2024-12-20T15:40:00Z

ambry-store/src/main/java/com/github/ambry/store/ReplicationProtocolTransitionType.java

@@ -0,0 +1,73 @@
+package com.github.ambry.store;
+
+public enum ReplicationProtocolTransitionType {


[Info] Proposing this way to thoroughly test BootstrapController's logic.
Tests are making use of these enum values and asserting if the logic takes an unexpected path.

ambry-store/src/main/java/com/github/ambry/store/BootstrapController.java

ambry-store/src/main/java/com/github/ambry/store/ReplicationProtocolTransitionType.java

justinlin-linkedin

LGTM, just some minor stuff.

ambry-store/src/main/java/com/github/ambry/store/BootstrapController.java

aga9900

LGTM

…otstrap-controller

DevenAhluwalia added 3 commits December 19, 2024 20:43

Init BootstrapController

010b715

(cherry picked from commit 1ce57b3)

Init BootstrapController

3bcb80e

(cherry picked from commit e991679)

NA

955f57c

DevenAhluwalia self-assigned this Dec 19, 2024

DevenAhluwalia mentioned this pull request Dec 19, 2024

[FileCopy Protocol] Bootstrap Controller #2960

Closed

NA

7d2aa65

DevenAhluwalia marked this pull request as ready for review December 20, 2024 06:19

aga9900 reviewed Dec 20, 2024

View reviewed changes

ambry-api/src/main/java/com/github/ambry/config/StoreConfig.java Outdated Show resolved Hide resolved

aga9900 reviewed Dec 20, 2024

View reviewed changes

ambry-api/src/main/java/com/github/ambry/config/StoreConfig.java Outdated Show resolved Hide resolved

aga9900 reviewed Dec 20, 2024

View reviewed changes

ambry-store/src/main/java/com/github/ambry/store/BootstrapController.java Outdated Show resolved Hide resolved

aga9900 reviewed Dec 20, 2024

View reviewed changes

ambry-api/src/main/java/com/github/ambry/server/StoreManager.java Outdated Show resolved Hide resolved

aga9900 reviewed Dec 20, 2024

View reviewed changes

ambry-clustermap/src/main/java/com/github/ambry/clustermap/HelixParticipant.java Outdated Show resolved Hide resolved

aga9900 reviewed Dec 20, 2024

View reviewed changes

ambry-store/src/main/java/com/github/ambry/store/BootstrapController.java Outdated Show resolved Hide resolved

aga9900 reviewed Dec 20, 2024

View reviewed changes

ambry-store/src/main/java/com/github/ambry/store/BootstrapController.java Outdated Show resolved Hide resolved

aga9900 reviewed Dec 20, 2024

View reviewed changes

ambry-store/src/main/java/com/github/ambry/store/BootstrapController.java Show resolved Hide resolved

aga9900 reviewed Dec 20, 2024

View reviewed changes

ambry-store/src/main/java/com/github/ambry/store/BootstrapController.java Show resolved Hide resolved

aga9900 reviewed Dec 20, 2024

View reviewed changes

ambry-store/src/main/java/com/github/ambry/store/BootstrapController.java Show resolved Hide resolved

aga9900 reviewed Dec 20, 2024

View reviewed changes

ambry-cloud/src/main/java/com/github/ambry/cloud/CloudStorageManager.java Show resolved Hide resolved

aga9900 reviewed Dec 20, 2024

View reviewed changes

ambry-cloud/src/main/java/com/github/ambry/cloud/CloudStorageManager.java Show resolved Hide resolved

aga9900 requested changes Dec 20, 2024

View reviewed changes

piyujai reviewed Dec 20, 2024

View reviewed changes

ambry-store/src/main/java/com/github/ambry/store/BootstrapController.java Outdated Show resolved Hide resolved

piyujai reviewed Dec 20, 2024

View reviewed changes

ambry-store/src/main/java/com/github/ambry/store/BootstrapController.java Outdated Show resolved Hide resolved

piyujai reviewed Dec 20, 2024

View reviewed changes

ambry-store/src/main/java/com/github/ambry/store/BootstrapController.java Outdated Show resolved Hide resolved

piyujai reviewed Dec 20, 2024

View reviewed changes

ambry-store/src/main/java/com/github/ambry/store/StorageManager.java Outdated Show resolved Hide resolved

aga9900 reviewed Dec 20, 2024

View reviewed changes

DevenAhluwalia added 2 commits December 20, 2024 16:31

Incorporated PR review comments

b1e001d

++ Test suite

bdd7024

DevenAhluwalia commented Dec 20, 2024

View reviewed changes

ambry-store/src/main/java/com/github/ambry/store/BootstrapController.java Outdated Show resolved Hide resolved

ambry-store/src/main/java/com/github/ambry/store/ReplicationProtocolTransitionType.java Outdated Show resolved Hide resolved

PR comments WIP

1e6cefa

justinlin-linkedin approved these changes Dec 20, 2024

View reviewed changes

aga9900 approved these changes Dec 20, 2024

View reviewed changes

DevenAhluwalia added 3 commits December 21, 2024 02:16

PR comments WIP

0b9cb66

PR comments WIP

a0ac6d7

Merge branch 'master' of https://github.com/linkedin/ambry into dw-bo…

84de042

…otstrap-controller

DevenAhluwalia merged commit 31f2dc3 into master Dec 20, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FileCopy Protocol] Bootstrap Controller #2974

[FileCopy Protocol] Bootstrap Controller #2974

DevenAhluwalia commented Dec 19, 2024 •

edited

Loading

aga9900 Dec 20, 2024

piyujai Dec 20, 2024

DevenAhluwalia Dec 20, 2024 •

edited

Loading

aga9900 Dec 20, 2024 •

edited

Loading

DevenAhluwalia Dec 20, 2024

DevenAhluwalia Dec 20, 2024

aga9900 left a comment

piyujai Dec 20, 2024

DevenAhluwalia Dec 20, 2024 •

edited

Loading

piyujai Dec 20, 2024

DevenAhluwalia Dec 20, 2024

piyujai Dec 20, 2024

DevenAhluwalia Dec 20, 2024

piyujai Dec 20, 2024

aga9900 Dec 20, 2024

DevenAhluwalia Dec 20, 2024

DevenAhluwalia Dec 20, 2024

DevenAhluwalia Dec 20, 2024

justinlin-linkedin left a comment

aga9900 left a comment



		// Permits Replica to be constructed with a null Partition
		public class TestReplica extends Replica {

		@@ -0,0 +1,73 @@
		package com.github.ambry.store;

		public enum ReplicationProtocolTransitionType {

[FileCopy Protocol] Bootstrap Controller #2974

[FileCopy Protocol] Bootstrap Controller #2974

Conversation

DevenAhluwalia commented Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DevenAhluwalia Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

aga9900 Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aga9900 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DevenAhluwalia Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinlin-linkedin left a comment

Choose a reason for hiding this comment

aga9900 left a comment

Choose a reason for hiding this comment

DevenAhluwalia commented Dec 19, 2024 •

edited

Loading

DevenAhluwalia Dec 20, 2024 •

edited

Loading

aga9900 Dec 20, 2024 •

edited

Loading

DevenAhluwalia Dec 20, 2024 •

edited

Loading