Introduce clustermap change listener to propagate remote replica addition/removal events #1368

jsjtzyy · 2020-01-28T00:14:06Z

With "move replica", Helix cluster manager should be able to dynamically add/remove remote replica infos and therefore clustermap will be dynamically changed. External components like ReplicationManager needs to update thread assignment and some in-mem data structures in repsonse to such changes. This PR introduces clustermap change listener which allows ReplicationManager to listen to any change in
clustermap and take actions accordingly. For now, there is only one method in clustermap
change listener: onReplicaAddedOrRemoved(), we could add more methods like onReplicaLeadershipChange etc if needed.

Remote replica addition/removal in clustermap is currently supported by dynamic cluster change handler only.( #1355 )

codecov-io · 2020-01-28T00:38:56Z

Codecov Report

Merging #1368 into master will increase coverage by <.01%.
The diff coverage is 84.37%.

@@             Coverage Diff              @@
##             master    #1368      +/-   ##
============================================
+ Coverage     72.08%   72.09%   +<.01%     
- Complexity     6792     6805      +13     
============================================
  Files           489      489              
  Lines         38530    38651     +121     
  Branches       4902     4920      +18     
============================================
+ Hits          27775    27865      +90     
- Misses         9417     9437      +20     
- Partials       1338     1349      +11

Impacted Files	Coverage Δ	Complexity Δ
...om.github.ambry.replication/ReplicationEngine.java	`79.04% <ø> (ø)`	`47 <0> (ø)`	⬇️
....github.ambry.clustermap/StaticClusterManager.java	`71.02% <0%> (-0.3%)`	`83 <0> (ø)`
...thub.ambry.clustermap/CompositeClusterManager.java	`59.72% <0%> (-1.71%)`	`22 <0> (ø)`
...m.github.ambry.clustermap/HelixClusterManager.java	`87.08% <33.33%> (-0.49%)`	`57 <0> (ø)`
...va/com.github.ambry.replication/PartitionInfo.java	`80% <76%> (-10.91%)`	`8 <5> (+3)`
...m.github.ambry.replication/ReplicationMetrics.java	`94.48% <85.71%> (-0.5%)`	`46 <5> (+3)`
...m.github.ambry.replication/ReplicationManager.java	`91.89% <95.89%> (+3.43%)`	`19 <4> (+2)`	⬆️
...in/java/com.github.ambry.clustermap/Partition.java	`76.92% <0%> (-3.3%)`	`28% <0%> (-1%)`
...github.ambry.rest/AsyncRequestResponseHandler.java	`87.87% <0%> (-2.36%)`	`21% <0%> (ø)`
...java/com.github.ambry.network/SSLTransmission.java	`70.46% <0%> (-0.93%)`	`76% <0%> (ø)`
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bfb751b...7310645. Read the comment docs.

ankagrawal · 2020-02-03T17:31:54Z

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

@@ -153,6 +153,11 @@ public MetricRegistry getMetricRegistry() {
    return metricRegistry;
  }

+  @Override
+  public void registerClusterMapListener(ClusterMapChangeListener clusterMapChangeListener) {
+    // no op for static cluster map.


Since we dont really support, do you think throwing an exception might work here?

sure, will make the change

I think this isnt addressed yet.

ambry-replication/src/main/java/com.github.ambry.replication/ReplicationManager.java

ankagrawal · 2020-02-03T18:08:56Z

ambry-replication/src/main/java/com.github.ambry.replication/ReplicationManager.java

+  @Override
+  public void onReplicaAddedOrRemoved(List<ReplicaId> addedReplicas, List<ReplicaId> removedReplicas) {
+    // 1. wait for start() to complete
+    try {


Also it looks like we have some duplicate code between this method and addReplica/removeReplica methods. Can we try to refactor?

addReplica(), removeReplica() are methods for changing replicas on local node. They create a new PartitionInfo along with all peer RemoteReplicaInfos. onReplicaAddedOrRemoved() basically modifies existing PartitionInfo by adding/removing certain RemoteReplicaInfo. Refactoring will be considered in a separate PR after we have verified functionality of this change.

justinlin-linkedin · 2020-02-04T23:47:06Z

ambry-replication/src/main/java/com.github.ambry.replication/ReplicationManager.java

+   * concurrently update remote replica infos.
+   */
+  @Override
+  public void onReplicaAddedOrRemoved(List<ReplicaId> addedReplicas, List<ReplicaId> removedReplicas) {


I suggest adding a private class to serve as the implementation of ClusterMapChangeListener, just like PartitionStateChangeListener.

Any benefits we can get by doing so? One probably reason I come up with is: when unifying all existing replication managers into a general replication manager, it requires 2 clustermap change listeners taking actions against changes in VCR and regular Ambry cluster. Feel free to add more thoughts here.

justinlin-linkedin · 2020-02-05T00:21:36Z

ambry-replication/src/main/java/com.github.ambry.replication/PartitionInfo.java

+   * @return {@code true} if remote replica info is added. {@code false} if it is already present
+   */
+  boolean addReplicaInfoIfAbsent(RemoteReplicaInfo remoteReplicaInfo) {
+    lock.lock();


please lock getRemoveReplicaInfos method as well, this lock is protecting remoteReplicas list, we should lock it everywhere we use it.

make sense, i will use read-write lock instead

justinlin-linkedin · 2020-02-05T00:27:25Z

ambry-replication/src/main/java/com.github.ambry.replication/ReplicationManager.java

+          if (partitionInfo.addReplicaInfoIfAbsent(remoteReplicaInfo)) {
+            replicationMetrics.addMetricsForRemoteReplicaInfo(remoteReplicaInfo);
+          }
+          replicaInfosToAdd.add(remoteReplicaInfo);


we should only add remoteReplicaInfo to the "toAdd" list when it was successfully added to partitionInfo, so we should move this statement in the if-else statement above.

Correct, will fix this.

justinlin-linkedin · 2020-02-05T00:37:43Z

ambry-replication/src/main/java/com.github.ambry.replication/ReplicationManager.java

+        for (ReplicaId remoteReplica : removedPeerReplicas) {
+          PartitionInfo partitionInfo = partitionToPartitionInfo.get(remoteReplica.getPartitionId());
+          List<RemoteReplicaInfo> remoteReplicaInfos = new ArrayList<>(partitionInfo.getRemoteReplicaInfos());
+          for (RemoteReplicaInfo remoteReplicaInfo : remoteReplicaInfos) {
+            if (remoteReplicaInfo.getReplicaId().getDataNodeId() == remoteReplica.getDataNodeId()) {
+              logger.info("Removing remote replica {} on {} from replica threads.", remoteReplica.getReplicaPath(),
+                  remoteReplica.getDataNodeId());
+              replicaInfosToRemove.add(remoteReplicaInfo);
+              if (partitionInfo.removeRelicaInfoIfPresent(remoteReplicaInfo)) {
+                replicationMetrics.removeMetricsForRemoteReplicaInfo(remoteReplicaInfo);
+              }
+              break;
+            }
+          }
+        }


why do we need this? we can just change the PartitionInfo's remove method to take a replica id as parameter, and looping the list in PartitionInfo, it's more aligned with addReplicaInfo.

right, will change removeRelicaInfoIfPresent

justinlin-linkedin · 2020-02-05T00:38:27Z

ambry-replication/src/main/java/com.github.ambry.replication/ReplicationManager.java

+            if (remoteReplicaInfo.getReplicaId().getDataNodeId() == remoteReplica.getDataNodeId()) {
+              logger.info("Removing remote replica {} on {} from replica threads.", remoteReplica.getReplicaPath(),
+                  remoteReplica.getDataNodeId());
+              replicaInfosToRemove.add(remoteReplicaInfo);


And we should put this statement within if-else statement as well.

events With "move replica", Helix cluster manager should be able to dynamically add/remove remote replica infos and therefore clustermap will be dynamically changed. External components like ReplicationManager needs to update thread assignment and some in-mem data structures in repsonse to such changes. This PR introduces clustermap change listener which allows ReplicationManager to listen to any change in clustermap and take actions accordingly. For now, there is only one method in clustermap change listener: onReplicaAddedOrRemoved(), we could add more methods like onReplicaLeadershipChange etc if needed.

ankagrawal

Looks good to me overall.

jsjtzyy self-assigned this Jan 28, 2020

jsjtzyy changed the title ~~WIP: Introduce clustermap change listener to propagate remote replica addition/removal events~~ Introduce clustermap change listener to propagate remote replica addition/removal events Jan 29, 2020

jsjtzyy marked this pull request as ready for review January 29, 2020 06:12

jsjtzyy requested a review from ankagrawal January 29, 2020 19:53

ankagrawal reviewed Feb 3, 2020

View reviewed changes

jsjtzyy requested a review from justinlin-linkedin February 3, 2020 21:11

justinlin-linkedin reviewed Feb 5, 2020

View reviewed changes

jsjtzyy added 6 commits February 6, 2020 17:44

add some tests and remove replica from metrics

ccbe492

add more test cases

daeceec

improve test coverage

977da23

comments

c4123b7

rebase and fix conflicts

fa026e9

jsjtzyy force-pushed the clustermap-callback branch from b12d085 to fa026e9 Compare February 7, 2020 02:14

one more test case

e8c6f9d

ankagrawal approved these changes Feb 10, 2020

View reviewed changes

minor comment

7310645

justinlin-linkedin approved these changes Feb 10, 2020

View reviewed changes

justinlin-linkedin merged commit bc91abd into linkedin:master Feb 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce clustermap change listener to propagate remote replica addition/removal events #1368

Introduce clustermap change listener to propagate remote replica addition/removal events #1368

jsjtzyy commented Jan 28, 2020 •

edited

Loading

codecov-io commented Jan 28, 2020 •

edited

Loading

ankagrawal Feb 3, 2020

jsjtzyy Feb 3, 2020

ankagrawal Feb 10, 2020

ankagrawal Feb 3, 2020

jsjtzyy Feb 6, 2020

justinlin-linkedin Feb 4, 2020

jsjtzyy Feb 6, 2020

justinlin-linkedin Feb 5, 2020

jsjtzyy Feb 6, 2020

justinlin-linkedin Feb 5, 2020

jsjtzyy Feb 6, 2020

justinlin-linkedin Feb 5, 2020

jsjtzyy Feb 7, 2020

justinlin-linkedin Feb 5, 2020

jsjtzyy Feb 6, 2020

ankagrawal left a comment

Introduce clustermap change listener to propagate remote replica addition/removal events #1368

Introduce clustermap change listener to propagate remote replica addition/removal events #1368

Conversation

jsjtzyy commented Jan 28, 2020 • edited Loading

codecov-io commented Jan 28, 2020 • edited Loading

Codecov Report

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

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

ankagrawal left a comment

Choose a reason for hiding this comment

jsjtzyy commented Jan 28, 2020 •

edited

Loading

codecov-io commented Jan 28, 2020 •

edited

Loading