You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/03/06 04:36:38 UTC
[GitHub] [helix] alirezazamani opened a new pull request #872: Change the
cluster creation logic
alirezazamani opened a new pull request #872: Change the cluster creation logic
URL: https://github.com/apache/helix/pull/872
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
Fixes #871
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
In this PR, a minor modification has been added which:
1- Populated the Topology information in the Cluster Config
if cloud is enabled.
2- Tests have been change accordingly.
3- The deprecated APIs have been changed in the test to
follow new APIs.
### Tests
- [x] The following is the result of the "mvn test" command on the appropriate module:
Helix-core:
[INFO] Tests run: 906, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 3,585.59 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 906, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 59:51 min
[INFO] Finished at: 2020-03-05T20:13:30-08:00
[INFO] ------------------------------------------------------------------------
Helix-Rest:
[INFO] Tests run: 99, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 27.966 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 99, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 33.294 s
[INFO] Finished at: 2020-03-05T19:12:48-08:00
[INFO] ------------------------------------------------------------------------
### Commits
- [x] My commits all reference appropriate Apache Helix GitHub issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- [x] My diff has been formatted using helix-style.xml
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #872:
Change the cluster creation logic
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #872: Change the cluster creation logic
URL: https://github.com/apache/helix/pull/872#discussion_r388722646
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
##########
@@ -189,6 +189,16 @@ public void addCluster(String clusterName, boolean overwritePrevious, CloudConfi
if (cloudConfig != null) {
_admin.addCloudConfig(clusterName, cloudConfig);
+ // If cloud is enabled and Cloud Provider is Azure, populated the Topology information in cluster config
+ if (cloudConfig.isCloudEnabled()
Review comment:
1. Can we update the cloudconfig first then add it to the cluster? So that we have only one ZK access.
2. If the following config is required for AZURE, shall we add it to a more fundamental method instead of the tool? What if the user call the other API or admin method to create a cluster that is not compatible?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc merged pull request #872: Change the cluster
creation logic
Posted by GitBox <gi...@apache.org>.
dasahcc merged pull request #872: Change the cluster creation logic
URL: https://github.com/apache/helix/pull/872
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #872:
Change the cluster creation logic
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #872: Change the cluster creation logic
URL: https://github.com/apache/helix/pull/872#discussion_r389045096
##########
File path: helix-core/src/test/java/org/apache/helix/tools/TestClusterSetup.java
##########
@@ -355,7 +356,7 @@ public void testDropInstance() throws Exception {
// add fake liveInstance
ZKHelixDataAccessor accessor =
- new ZKHelixDataAccessor(clusterName, new ZkBaseDataAccessor<ZNRecord>(_gZkClient));
+ new ZKHelixDataAccessor(clusterName, new ZkBaseDataAccessor<ZNRecord>(ZK_ADDR));
Review comment:
The method has been deprecated in November 2019. So I think it is almost recent.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #872:
Change the cluster creation logic
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #872: Change the cluster creation logic
URL: https://github.com/apache/helix/pull/872#discussion_r389047744
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
##########
@@ -189,6 +189,16 @@ public void addCluster(String clusterName, boolean overwritePrevious, CloudConfi
if (cloudConfig != null) {
_admin.addCloudConfig(clusterName, cloudConfig);
+ // If cloud is enabled and Cloud Provider is Azure, populated the Topology information in cluster config
+ if (cloudConfig.isCloudEnabled()
Review comment:
1. Please note that we are updating two different paths. The new information will be added to cluster config. So eventually we need two accesses. one for the cloud config and one for the cluster config.
2. Our plan is to populate these information while creating the cluster and all of the paths (REST and java) to create cluster should pass through this part of the code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on issue #872: Change the cluster
creation logic
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on issue #872: Change the cluster creation logic
URL: https://github.com/apache/helix/pull/872#issuecomment-595945420
This PR is ready to be merged, approved by @dasahcc and @jiajunwang
FInal Commit message:
Title:
Add cluster config update for cluster creation logic in Azure
Body:
In this commit, minor modifications have been added which:
1- Update topology information in the Cluster Config
if cloud is enabled for Azure environment.
2- Tests have been change accordingly.
3- The deprecated APIs have been changed in the test to
follow new APIs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #872: Change
the cluster creation logic
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #872: Change the cluster creation logic
URL: https://github.com/apache/helix/pull/872#discussion_r389049506
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
##########
@@ -189,6 +189,16 @@ public void addCluster(String clusterName, boolean overwritePrevious, CloudConfi
if (cloudConfig != null) {
_admin.addCloudConfig(clusterName, cloudConfig);
+ // If cloud is enabled and Cloud Provider is Azure, populated the Topology information in cluster config
+ if (cloudConfig.isCloudEnabled()
+ && cloudConfig.getCloudProvider().equals(CloudProvider.AZURE.name())) {
+ ConfigAccessor configAccessor = new ConfigAccessor(_zkServerAddress);
+ ClusterConfig clusterConfig = new ClusterConfig(clusterName);
+ clusterConfig.setTopology("/helixZoneId/host");
Review comment:
Are we agree use this? I thought i was "/faultDomain/hostname"? Anyway, we should put it into AzureConstant.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #872:
Change the cluster creation logic
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #872: Change the cluster creation logic
URL: https://github.com/apache/helix/pull/872#discussion_r389096088
##########
File path: helix-core/src/main/java/org/apache/helix/tools/ClusterSetup.java
##########
@@ -189,6 +189,16 @@ public void addCluster(String clusterName, boolean overwritePrevious, CloudConfi
if (cloudConfig != null) {
_admin.addCloudConfig(clusterName, cloudConfig);
+ // If cloud is enabled and Cloud Provider is Azure, populated the Topology information in cluster config
+ if (cloudConfig.isCloudEnabled()
+ && cloudConfig.getCloudProvider().equals(CloudProvider.AZURE.name())) {
+ ConfigAccessor configAccessor = new ConfigAccessor(_zkServerAddress);
+ ClusterConfig clusterConfig = new ClusterConfig(clusterName);
+ clusterConfig.setTopology("/helixZoneId/host");
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #872:
Change the cluster creation logic
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #872: Change the cluster creation logic
URL: https://github.com/apache/helix/pull/872#discussion_r388722821
##########
File path: helix-core/src/test/java/org/apache/helix/tools/TestClusterSetup.java
##########
@@ -355,7 +356,7 @@ public void testDropInstance() throws Exception {
// add fake liveInstance
ZKHelixDataAccessor accessor =
- new ZKHelixDataAccessor(clusterName, new ZkBaseDataAccessor<ZNRecord>(_gZkClient));
+ new ZKHelixDataAccessor(clusterName, new ZkBaseDataAccessor<ZNRecord>(ZK_ADDR));
Review comment:
Question, is this because of the new ZK change? Or other reasons?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org