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 21:48:53 UTC
[GitHub] [helix] zhangmeng916 opened a new pull request #876: Helix domain
zhangmeng916 opened a new pull request #876: Helix domain
URL: https://github.com/apache/helix/pull/876
### Issues
- [ ] My PR addresses the following Helix issues and references them in the PR description:
(#875 )
### Description
- [X] Here are some details about my PR, including screenshots of any UI changes:
Helix has a topology defined in cluster config, e.g., "TOPOLOGY": "/zone/host", and it will be used to construct the "domain" field in participant config, e.g. ""DOMAIN": "zone=0,host=host1". In Azure, Helix will provide a default topology value for the cluster, and the format is /faultDomain/hostName. In this pR, we add the construction function in Participant manager to construct the domain field based on this topology.
### Tests
- [ ] The following tests are written for this issue:
(List the names of added unit/integration tests)
- [X] The following is the result of the "mvn test" command on the appropriate module:
### Commits
- [ ] My commits all reference appropriate Apache Helix GitHub issues in their subject lines. 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"
### Documentation (Optional)
- [ ] In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Code Quality
- [X ] My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
----------------------------------------------------------------
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 #876: Add
construction of domain in Helix participant logic
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #876: Add construction of domain in Helix participant logic
URL: https://github.com/apache/helix/pull/876#discussion_r389902393
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureCloudInstanceInformationProcessor.java
##########
@@ -142,8 +142,16 @@ public AzureCloudInstanceInformation parseCloudInstanceInformation(List<String>
String vmName = computeNode.path(INSTANCE_NAME).getTextValue();
String platformFaultDomain = computeNode.path(DOMAIN).getTextValue();
String vmssName = computeNode.path(INSTANCE_SET_NAME).getValueAsText();
+ String azureTopology = AzureConstants.AZURE_TOPOLOGY;
+ String[] parts = azureTopology.trim().split("/");
+ if (parts.length != 2) {
+ throw new HelixException("Invalid Azure topology definition: " + azureTopology);
+ }
Review comment:
This is not necessary, right? Unless we offer flexibility for user to customize it.
----------------------------------------------------------------
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] zhangmeng916 commented on a change in pull request #876:
Add construction of domain in Helix participant logic
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #876: Add construction of domain in Helix participant logic
URL: https://github.com/apache/helix/pull/876#discussion_r389179074
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ParticipantManager.java
##########
@@ -200,6 +201,22 @@ private void joinCluster() {
}
}
+ private String ConstructDomainField(String faultDomain) {
+ Boolean topologyEnabled = _configAccessor.getClusterConfig(_clusterName).isTopologyAwareEnabled();
Review comment:
In this logic, we do not parse the cloud provider directly. That logic is in the processor. I was thinking that if we read from clusterConfig, it'll be generic, and independent of cloud provider. thoughts?
----------------------------------------------------------------
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 #876: Helix
domain
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #876: Helix domain
URL: https://github.com/apache/helix/pull/876#discussion_r389168774
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ParticipantManager.java
##########
@@ -200,6 +201,22 @@ private void joinCluster() {
}
}
+ private String ConstructDomainField(String faultDomain) {
+ Boolean topologyEnabled = _configAccessor.getClusterConfig(_clusterName).isTopologyAwareEnabled();
Review comment:
Do we need this? If we know the environment is Azure, then we can directly get the topology from AzureConstants instead of reading from ClusterConfig.
----------------------------------------------------------------
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] zhangmeng916 commented on a change in pull request #876:
Add construction of domain in Helix participant logic
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #876: Add construction of domain in Helix participant logic
URL: https://github.com/apache/helix/pull/876#discussion_r389924781
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ParticipantManager.java
##########
@@ -178,7 +178,7 @@ private void joinCluster() {
LOG.info(_instanceName + " is auto-registering cluster: " + _clusterName);
CloudInstanceInformation cloudInstanceInformation = getCloudInstanceInformation();
String domain = cloudInstanceInformation
- .get(CloudInstanceInformation.CloudInstanceField.FAULT_DOMAIN.name());
+ .get(CloudInstanceInformation.CloudInstanceField.FAULT_DOMAIN.name()) + _instanceName;
Review comment:
The prefix is in the processor:
//The hostname will be filled in by each participant
String domain = parts[0] + "=" + platformFaultDomain + "," + parts[1] + "=";
----------------------------------------------------------------
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] zhangmeng916 commented on issue #876: Add construction of
domain in Helix participant logic
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on issue #876: Add construction of domain in Helix participant logic
URL: https://github.com/apache/helix/pull/876#issuecomment-596810333
This PR is ready to merge, approved by @dasahcc
Final commit message:
Add the construction of domain field in Azure cloud instance information processor. It will read the topology from AzureConstants, and construct the domain based on the format of the topology.
----------------------------------------------------------------
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 #876: Add
construction of domain in Helix participant logic
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #876: Add construction of domain in Helix participant logic
URL: https://github.com/apache/helix/pull/876#discussion_r389973302
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureCloudInstanceInformationProcessor.java
##########
@@ -142,8 +142,16 @@ public AzureCloudInstanceInformation parseCloudInstanceInformation(List<String>
String vmName = computeNode.path(INSTANCE_NAME).getTextValue();
String platformFaultDomain = computeNode.path(DOMAIN).getTextValue();
String vmssName = computeNode.path(INSTANCE_SET_NAME).getValueAsText();
+ String azureTopology = AzureConstants.AZURE_TOPOLOGY;
+ String[] parts = azureTopology.trim().split("/");
+ if (parts.length != 2) {
+ throw new HelixException("Invalid Azure topology definition: " + azureTopology);
+ }
Review comment:
But this is not from the file. This is from Java class AzureConstants.
----------------------------------------------------------------
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 #876: Add
construction of domain in Helix participant logic
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #876: Add construction of domain in Helix participant logic
URL: https://github.com/apache/helix/pull/876#discussion_r389903510
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ParticipantManager.java
##########
@@ -178,7 +178,7 @@ private void joinCluster() {
LOG.info(_instanceName + " is auto-registering cluster: " + _clusterName);
CloudInstanceInformation cloudInstanceInformation = getCloudInstanceInformation();
String domain = cloudInstanceInformation
- .get(CloudInstanceInformation.CloudInstanceField.FAULT_DOMAIN.name());
+ .get(CloudInstanceInformation.CloudInstanceField.FAULT_DOMAIN.name()) + _instanceName;
Review comment:
This is domain string right? Then it should looked like "FD=xxx, InstanceName=xxxx". But the code is adding them together?
----------------------------------------------------------------
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] zhangmeng916 commented on a change in pull request #876:
Add construction of domain in Helix participant logic
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #876: Add construction of domain in Helix participant logic
URL: https://github.com/apache/helix/pull/876#discussion_r389975285
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureCloudInstanceInformationProcessor.java
##########
@@ -142,8 +142,16 @@ public AzureCloudInstanceInformation parseCloudInstanceInformation(List<String>
String vmName = computeNode.path(INSTANCE_NAME).getTextValue();
String platformFaultDomain = computeNode.path(DOMAIN).getTextValue();
String vmssName = computeNode.path(INSTANCE_SET_NAME).getValueAsText();
+ String azureTopology = AzureConstants.AZURE_TOPOLOGY;
+ String[] parts = azureTopology.trim().split("/");
+ if (parts.length != 2) {
+ throw new HelixException("Invalid Azure topology definition: " + azureTopology);
+ }
Review comment:
oh yes, I was thinking about my property file. Removed.
----------------------------------------------------------------
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] zhangmeng916 commented on a change in pull request #876:
Add construction of domain in Helix participant logic
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #876: Add construction of domain in Helix participant logic
URL: https://github.com/apache/helix/pull/876#discussion_r389925050
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureCloudInstanceInformationProcessor.java
##########
@@ -142,8 +142,16 @@ public AzureCloudInstanceInformation parseCloudInstanceInformation(List<String>
String vmName = computeNode.path(INSTANCE_NAME).getTextValue();
String platformFaultDomain = computeNode.path(DOMAIN).getTextValue();
String vmssName = computeNode.path(INSTANCE_SET_NAME).getValueAsText();
+ String azureTopology = AzureConstants.AZURE_TOPOLOGY;
+ String[] parts = azureTopology.trim().split("/");
+ if (parts.length != 2) {
+ throw new HelixException("Invalid Azure topology definition: " + azureTopology);
+ }
Review comment:
I was thinking whether it's possible the file was accidentally changed.
----------------------------------------------------------------
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] zhangmeng916 commented on a change in pull request #876:
Add construction of domain in Helix participant logic
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #876: Add construction of domain in Helix participant logic
URL: https://github.com/apache/helix/pull/876#discussion_r389217116
##########
File path: helix-core/src/main/java/org/apache/helix/manager/zk/ParticipantManager.java
##########
@@ -200,6 +201,22 @@ private void joinCluster() {
}
}
+ private String ConstructDomainField(String faultDomain) {
+ Boolean topologyEnabled = _configAccessor.getClusterConfig(_clusterName).isTopologyAwareEnabled();
Review comment:
Discussed offline. We'll move the logic to cloud instance information processor and not to depend on cluster config.
----------------------------------------------------------------
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 #876: Add construction of
domain in Helix participant logic
Posted by GitBox <gi...@apache.org>.
dasahcc merged pull request #876: Add construction of domain in Helix participant logic
URL: https://github.com/apache/helix/pull/876
----------------------------------------------------------------
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