Allow ambry server to update replica info in Helix InstanceConfig #1348

jsjtzyy · 2019-12-30T01:28:29Z

Once replica addition or removal is complete on particular datanode,
ambry server is supposed to update InstanceConfig in Helix to broadcast
the replica related changes. This PR introduced a new method that allows
participant to update its node info in cluster. It supports adding new
replica info or removing old replica info. For rollback purpose, we also
add a config to switch on/off update operation.

codecov-io · 2019-12-30T01:50:12Z

Codecov Report

Merging #1348 into master will increase coverage by 0.04%.
The diff coverage is 94.68%.

@@             Coverage Diff              @@
##             master    #1348      +/-   ##
============================================
+ Coverage     71.98%   72.03%   +0.04%     
- Complexity     6680     6696      +16     
============================================
  Files           486      486              
  Lines         38070    38163      +93     
  Branches       4826     4843      +17     
============================================
+ Hits          27406    27490      +84     
- Misses         9354     9361       +7     
- Partials       1310     1312       +2

Impacted Files	Coverage Δ	Complexity Δ
...b.ambry.clustermap/StaticClusterAgentsFactory.java	`31.57% <0%> (-0.86%)`	`4 <0> (ø)`
...hub.ambry/clustermap/StateTransitionException.java	`100% <100%> (ø)`	`2 <0> (ø)`	⬇️
...java/com.github.ambry/config/ClusterMapConfig.java	`100% <100%> (ø)`	`3 <0> (ø)`	⬇️
...in/java/com.github.ambry.store/StorageManager.java	`88.31% <87.5%> (-0.04%)`	`54 <0> (ø)`
.../com.github.ambry.clustermap/HelixParticipant.java	`85.55% <96.34%> (+4.88%)`	`56 <19> (+19)`	⬆️
...github.ambry.rest/AsyncRequestResponseHandler.java	`87.87% <0%> (-2.36%)`	`21% <0%> (ø)`
...c/main/java/com.github.ambry.network/Selector.java	`78.21% <0%> (-0.79%)`	`77% <0%> (-1%)`
...rc/main/java/com.github.ambry.store/BlobStore.java	`89.93% <0%> (-0.23%)`	`103% <0%> (-1%)`
...in/java/com.github.ambry.store/BlobStoreStats.java	`70.41% <0%> (ø)`	`101% <0%> (-2%)`	⬇️
.../java/com.github.ambry.router/DeleteOperation.java	`94.89% <0%> (+1.45%)`	`45% <0%> (+1%)`	⬆️
... and 1 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 dccd32c...c559a6d. Read the comment docs.

zzmao

Will take a second review on addNewReplicaInfo and removeOldReplicaInfo

zzmao · 2020-01-07T19:53:20Z

ambry-api/src/main/java/com.github.ambry/clustermap/ClusterParticipant.java

+   * Update disk/replica infos associated with current data node in cluster (this occurs when replica addition/removal
+   * on current node is complete and local changes will be broadcast to all listeners in this cluster)
+   * @param replicaId the {@link ReplicaId} whose info should be updated on current node
+   * @param shouldPresent Whether the replica info should be present. When {@code true}, replica info will be added if


Minor: Feel addOrRemove is better.

why not have two methods: addDataNodeInfoInCluster and removeDataNodeInfoFromCluster?

addOrRemove seems to require an enum. We cannot tell if it should be added or removed by looking at the boolean value (true/false). Aside from shouldPresent, I also consider shouldExist. I am still open to other proper names.

shouldExist feels better.

ok, will make the change.

@ankagrawal I guess you are suggesting addReplicaToDataNodeInfo and removeReplicaFromDataNodeInfo. Yes, we could but a single method with boolean variable would suffice and seems to be more concise. In addition, updating data node info only applies to HelixParticipant. That is, we don't need to introduce one more unnecessary method to other implementation of ClusterParticipant (i.e. StaticClusterParticipant).

zzmao · 2020-01-15T21:17:15Z

config/server.properties

@@ -16,3 +16,4 @@ host.name=localhost
 clustermap.cluster.name=Ambry_Dev
 clustermap.datacenter.name=Datacenter
 clustermap.host.name=localhost
+clustermap.port=6667


Why is this newly added?

ah, I just added it here to fix issue #1357 (probably should be in a separate PR but let's get it fixed together with this PR merged)

Feel free to ignore this, not related to this PR.

It's OK. I also need this change. :)

Thanks for fixing this.

zzmao · 2020-01-17T19:57:34Z

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

+        for (String replicaInfo : replicaInfos) {
+          String[] infos = replicaInfo.split(ClusterMapUtils.REPLICAS_STR_SEPARATOR);
+          if (infos[0].equals(partitionName)) {
+            replicaFound = true;


split string to arrayList and then removeIf probably needs less code.

Good point, will make the change.

zzmao

LGTM. One comment left.

jsjtzyy · 2020-01-21T18:47:44Z

@ankagrawal gentle reminder to review, thanks!

ankagrawal · 2020-01-13T20:48:31Z

ambry-api/src/main/java/com.github.ambry/clustermap/ClusterParticipant.java

+   * Update disk/replica infos associated with current data node in cluster (this occurs when replica addition/removal
+   * on current node is complete and local changes will be broadcast to all listeners in this cluster)
+   * @param replicaId the {@link ReplicaId} whose info should be updated on current node
+   * @param shouldPresent Whether the replica info should be present. When {@code true}, replica info will be added if


why not have two methods: addDataNodeInfoInCluster and removeDataNodeInfoFromCluster?

ankagrawal · 2020-01-28T17:54:20Z

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

+  private boolean addNewReplicaInfo(ReplicaId replicaId, InstanceConfig instanceConfig) {
+    boolean additionResult = true;
+    String partitionName = replicaId.getPartitionId().toPathString();
+    String newReplicaInfo = partitionName + ClusterMapUtils.REPLICAS_STR_SEPARATOR + replicaId.getCapacityInBytes()


Minor:
Maybe try String.join(ClusterMapUtils.REPLICAS_STR_SEPARATOR, partitionName, replicaId.getCapacityInBytes(), replicaId.getPartitionId().getPartitionClass()) + ClusterMapUtils.REPLICAS_DELIM_STR; ?

sure, let me make the change.

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

ankagrawal · 2020-01-28T18:32:53Z

config/server.properties

@@ -16,3 +16,4 @@ host.name=localhost
 clustermap.cluster.name=Ambry_Dev
 clustermap.datacenter.name=Datacenter
 clustermap.host.name=localhost
+clustermap.port=6667


Thanks for fixing this.

ankagrawal

LGTM.
Address the minor comment to add the log and then I will merge it after travis is successful.

jsjtzyy self-assigned this Dec 30, 2019

jsjtzyy changed the title ~~WIP: Allow ambry server to update replica info in Helix InstanceConfig~~ Allow ambry server to update replica info in Helix InstanceConfig Jan 1, 2020

jsjtzyy marked this pull request as ready for review January 1, 2020 06:50

jsjtzyy requested review from ankagrawal and zzmao January 7, 2020 17:44

zzmao reviewed Jan 7, 2020

View reviewed changes

jsjtzyy mentioned this pull request Jan 13, 2020

NullPointerException when starting ambry-server locally with static cluster manager #1357

Closed

jsjtzyy force-pushed the update-instance-config branch from 7cc6be2 to f679e74 Compare January 15, 2020 21:03

zzmao reviewed Jan 15, 2020

View reviewed changes

zzmao reviewed Jan 17, 2020

View reviewed changes

zzmao approved these changes Jan 17, 2020

View reviewed changes

ankagrawal reviewed Jan 28, 2020

View reviewed changes

jsjtzyy added 3 commits January 28, 2020 15:31

rebase master

4ae5abc

comments

add1f8d

rebase and address comments

c559a6d

jsjtzyy force-pushed the update-instance-config branch from 896a1fe to c559a6d Compare January 29, 2020 02:32

ankagrawal approved these changes Jan 29, 2020

View reviewed changes

ankagrawal merged commit 906d0bb into linkedin:master Jan 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow ambry server to update replica info in Helix InstanceConfig #1348

Allow ambry server to update replica info in Helix InstanceConfig #1348

jsjtzyy commented Dec 30, 2019

codecov-io commented Dec 30, 2019 •

edited

Loading

zzmao left a comment

zzmao Jan 7, 2020

ankagrawal Jan 13, 2020

jsjtzyy Jan 13, 2020

zzmao Jan 13, 2020

jsjtzyy Jan 13, 2020

jsjtzyy Jan 28, 2020

zzmao Jan 15, 2020

jsjtzyy Jan 15, 2020

jsjtzyy Jan 15, 2020

zzmao Jan 15, 2020

ankagrawal Jan 28, 2020

zzmao Jan 17, 2020

jsjtzyy Jan 21, 2020

zzmao left a comment

jsjtzyy commented Jan 21, 2020

ankagrawal Jan 13, 2020

ankagrawal Jan 28, 2020

jsjtzyy Jan 28, 2020

ankagrawal Jan 28, 2020

ankagrawal left a comment

Allow ambry server to update replica info in Helix InstanceConfig #1348

Allow ambry server to update replica info in Helix InstanceConfig #1348

Conversation

jsjtzyy commented Dec 30, 2019

codecov-io commented Dec 30, 2019 • edited Loading

Codecov Report

zzmao left a 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

zzmao left a comment

Choose a reason for hiding this comment

jsjtzyy commented Jan 21, 2020

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

codecov-io commented Dec 30, 2019 •

edited

Loading