Skip to content
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

Upload new replicas infos onto Helix PropertyStore #1249

Merged
merged 5 commits into from
Sep 10, 2019
Merged

Upload new replicas infos onto Helix PropertyStore #1249

merged 5 commits into from
Sep 10, 2019

Conversation

jsjtzyy
Copy link
Contributor

@jsjtzyy jsjtzyy commented Aug 24, 2019 •
edited
Loading

This PR supports uploading infos of new replicas onto Helix
PropertyStore. This is part of move replica which specifies the location
of new replicas and their size, partition class etc. The Helix bootstrap
upgrade tool will extract diff between static clustermap and clustermap
in Helix. The diff is used to generate a map of new added replicas and
then upload it to Helix.

@jsjtzyy
Upload new added replicas info onto Helix PropertyStore
56cb657 
This PR supports uploading infos of new added replicas onto Helix
PropertyStore. This is part of move replica which specifies the location
of new replicas and their size, partition class etc. The Helix bootstrap
upgrade tool will extract diff between static clustermap and clustermap
in Helix. The diff is used to generate a map of new added replicas and
then upload it to Helix.
@codecov-io
Copy link

codecov-io commented Aug 24, 2019 •
edited
Loading

Codecov Report

Merging #1249 into master will increase coverage by 0.02%.
The diff coverage is 77.41%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1249      +/-   ##
============================================
+ Coverage     72.18%   72.21%   +0.02%     
- Complexity     6052     6076      +24     
============================================
  Files           439      439              
  Lines         34971    35071     +100     
  Branches       4437     4459      +22     
============================================
+ Hits          25245    25327      +82     
- Misses         8568     8580      +12     
- Partials       1158     1164       +6
Impacted Files Coverage Δ Complexity Δ
...ub.ambry/clustermap/HelixBootstrapUpgradeTool.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...hub.ambry.clustermap/AmbryPartitionStateModel.java 21.21% <0%> (ø) 1 <0> (ø) ⬇️
...a/com.github.ambry.clustermap/ClusterMapUtils.java 92.56% <100%> (ø) 36 <0> (ø) ⬇️
...m.github.ambry.clustermap/HelixClusterManager.java 86.25% <100%> (+0.03%) 42 <0> (ø) ⬇️
...ub.ambry.clustermap/HelixBootstrapUpgradeUtil.java 88.61% <86.07%> (-0.36%) 116 <14> (+16)
...github.ambry/config/HelixAccountServiceConfig.java 84.61% <0%> (-15.39%) 2% <0%> (+1%)
...rc/main/java/com.github.ambry.rest/RestServer.java 96.66% <0%> (-1.31%) 13% <0%> (ø)
...java/com.github.ambry.network/SSLTransmission.java 70.22% <0%> (-0.98%) 69% <0%> (-2%)
...ain/java/com.github.ambry.router/PutOperation.java 91.21% <0%> (-0.51%) 113% <0%> (-1%)
...in/java/com.github.ambry.network/SocketServer.java 83.88% <0%> (-0.42%) 15% <0%> (ø)
... and 5 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 306bb81...e63b79e. Read the comment docs.

@jsjtzyy jsjtzyy changed the title Upload new added replicas info onto Helix PropertyStore Upload new replicas infos onto Helix PropertyStore Aug 25, 2019
@jsjtzyy jsjtzyy self-assigned this Aug 25, 2019
justinlin-linkedin
justinlin-linkedin reviewed Aug 28, 2019
View reviewed changes
ambry-clustermap/src/main/java/com.github.ambry.clustermap/ClusterMapUtils.java
public static final String PROPERTYSTORE_ZNODE_PATH = "/PROPERTYSTORE/ClusterConfigs/" + ZNODE_NAME;
public static final String PARTITION_OVERRIDE_STR = "PartitionOverride";
public static final String REPLICA_ADDITION_STR = "ReplicaAddition";
public static final String PARTITION_OVERRIDE_ZNODE_PATH = "/AdminConfigs/" + PARTITION_OVERRIDE_STR;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add some comments to these three znode path, to describe what are the purpose of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, comments are added

ambry-clustermap/src/main/java/com.github.ambry.clustermap/HelixBootstrapUpgradeUtil.java
info("Uploading partition override to HelixPropertyStore based on override json file.");
clusterMapToHelixMapper.uploadPartitionOverride(partitionOverrideInfos);
info("Upload cluster configs completed.");
maxPartitionsInOneResource, false, false, helixAdminFactory, ClusterMapConfig.DEFAULT_STATE_MODEL_DEF);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why it's leaderstandby model? thought it should be the offline/boostrap/leader/standby?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will make it configurable. For now, we are still using default LeaderStandby model. To use new state model, we first need to create new model on Helix and make all participants register the new model.

ambry-clustermap/src/main/java/com.github.ambry.clustermap/ClusterMapUtils.java Outdated
public static final String REPLICA_ADDITION_STR = "ReplicaAddition";
public static final String PARTITION_OVERRIDE_ZNODE_PATH = "/AdminConfigs/" + PARTITION_OVERRIDE_STR;
public static final String REPLICA_ADDITION_ZNODE_PATH = "/AdminConfigs/" + REPLICA_ADDITION_STR;
public static final String PROPERTYSTORE_ZNODE_PATH = "/PROPERTYSTORE/AdminConfigs/";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the purpose of having this znode when we already have PARTITION_OVERRIDE_ZNODE_PATH?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair point, I removed PARTITION_OVERRIDE_ZNODE_PATH

justinlin-linkedin
justinlin-linkedin approved these changes Sep 2, 2019
View reviewed changes
Copy link
Collaborator

@justinlin-linkedin justinlin-linkedin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@jsjtzyy jsjtzyy mentioned this pull request Sep 9, 2019
Merged
@ankagrawal ankagrawal merged commit de3524c into linkedin:master Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinlin-linkedin justinlin-linkedin justinlin-linkedin approved these changes

@ankagrawal ankagrawal ankagrawal approved these changes

Assignees

@jsjtzyy jsjtzyy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jsjtzyy @codecov-io @ankagrawal @justinlin-linkedin