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/01/23 00:56:56 UTC
[GitHub] [helix] zhangmeng916 opened a new pull request #698: Implement
Azure cloud instance information processor
zhangmeng916 opened a new pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698
### Issues
- [X] My PR addresses the following Helix issues and references them in the PR description:
(#697 )
### Description
- [X] Here are some details about my PR, including screenshots of any UI changes:
Helix will provide a function for participant to auto register to the cluster so that no manual efforts are needed for adding instance to a cluster. To realize it, a cloud instance information processor will take the responsibility for getting and parsing the instance information from cloud. Different cloud providers will have different implementations for the processor. This PR implement the processor for Azure as a first use case.
### Tests
- [X] The following tests are written for this issue:
TestAzureCloudInstanceInformationProcessor
- [X] The following is the result of the "mvn test" command on the appropriate module:
[INFO] Tests run: 904, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 3,221.481 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 904, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 53:45 min
[INFO] Finished at: 2020-01-22T09:57:03-08:00
[INFO] ------------------------------------------------------------------------
[mnzhang@mnzhang-ld1 helix-core]$ git status
### 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
- [ ] 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] zhangmeng916 commented on a change in pull request #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r376165802
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureCloudInstanceInformationProcessor.java
##########
@@ -19,38 +19,135 @@
* under the License.
*/
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
import java.util.ArrayList;
import java.util.List;
+import javax.net.ssl.SSLException;
+import org.apache.helix.HelixCloudProperty;
+import org.apache.helix.HelixException;
import org.apache.helix.api.cloud.CloudInstanceInformationProcessor;
+import org.apache.http.client.HttpRequestRetryHandler;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.protocol.HttpContext;
+import org.apache.http.util.EntityUtils;
+import org.codehaus.jackson.JsonNode;
+import org.codehaus.jackson.map.ObjectMapper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+public class AzureCloudInstanceInformationProcessor
+ implements CloudInstanceInformationProcessor<String> {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(AzureCloudInstanceInformationProcessor.class);
+ private final CloseableHttpClient _closeableHttpClient;
+ private final HelixCloudProperty _helixCloudProperty;
+ private final String COMPUTE = "compute";
+ private final String INSTANCE_NAME = "vmId";
+ private final String DOMAIN = "platformFaultDomain";
+ private final String INSTANCE_SET_NAME = "vmScaleSetName";
-public class AzureCloudInstanceInformationProcessor implements CloudInstanceInformationProcessor<String> {
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty) {
+ _helixCloudProperty = helixCloudProperty;
- public AzureCloudInstanceInformationProcessor() {
+ RequestConfig requestConifg = RequestConfig.custom()
+ .setConnectionRequestTimeout((int) helixCloudProperty.getCloudRequestTimeout())
+ .setConnectTimeout((int) helixCloudProperty.getCloudConnectionTimeout()).build();
+
+ HttpRequestRetryHandler httpRequestRetryHandler =
+ (IOException exception, int executionCount, HttpContext context) -> {
+ LOG.warn("Execution count: " + executionCount + ".", exception);
+ return !(executionCount >= helixCloudProperty.getCloudMaxRetry()
+ || exception instanceof InterruptedIOException
+ || exception instanceof UnknownHostException || exception instanceof SSLException);
+ };
+
+ _closeableHttpClient = HttpClients.custom().setDefaultRequestConfig(requestConifg)
+ .setRetryHandler(httpRequestRetryHandler).build();
}
/**
- * fetch the raw Azure cloud instance information
+ * This constructor is for unit test purpose only.
+ * User could provide helixCloudProperty and a mocked http client to test the functionality of
+ * this class.
+ */
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty,
+ CloseableHttpClient closeableHttpClient) {
+ _helixCloudProperty = helixCloudProperty;
+ _closeableHttpClient = closeableHttpClient;
+ }
+
+ /**
+ * Fetch raw Azure cloud instance information based on the urls provided
* @return raw Azure cloud instance information
*/
@Override
public List<String> fetchCloudInstanceInformation() {
List<String> response = new ArrayList<>();
- //TODO: implement the fetching logic
+ for (String url : _helixCloudProperty.getCloudInfoSources()) {
+ response.add(getAzureCloudInformationFromUrl(url));
+ }
return response;
}
+ /**
+ * Query Azure Instance Metadata Service to get the instance(VM) information
+ * @return raw Azure cloud instance information
+ */
+ private String getAzureCloudInformationFromUrl(String url) {
+ HttpGet httpGet = new HttpGet(url);
+ httpGet.setHeader("Metadata", "true");
+
+ try {
+ CloseableHttpResponse response = _closeableHttpClient.execute(httpGet);
+ if (response == null || response.getStatusLine().getStatusCode() != 200) {
+ String errorMsg = String.format(
+ "Failed to get an HTTP Response for the request. Response: {}. Status code: {}",
+ (response == null ? "NULL" : response.getStatusLine().getReasonPhrase()),
+ response.getStatusLine().getStatusCode());
+ throw new HelixException(errorMsg);
+ }
+ String responseString = EntityUtils.toString(response.getEntity());
+ LOG.info("VM instance information query result: {}", responseString);
+ return responseString;
+ } catch (IOException e) {
+ throw new HelixException(
+ String.format("Failed to get Azure cloud instance information from url {}", url), e);
+ }
+ }
+
/**
* Parse raw Azure cloud instance information.
* @return required azure cloud instance information
*/
@Override
public AzureCloudInstanceInformation parseCloudInstanceInformation(List<String> responses) {
AzureCloudInstanceInformation azureCloudInstanceInformation = null;
- //TODO: implement the parsing logic
+ for (String response : responses) {
Review comment:
Discussed offline. Updated Azure implementation to restrict the response size to 1, as we do not have any source to query for now.
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r373212013
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureCloudInstanceInformation.java
##########
@@ -42,7 +42,13 @@ public String get(String key) {
}
public static class Builder {
- private Map<String, String> _cloudInstanceInfoMap = null;
+ /**
+ * Default constructor
+ */
+ public Builder() {
+ }
+
+ private Map<String, String> _cloudInstanceInfoMap = new HashMap<>();
Review comment:
Feel free to make it final if you are not going to create a new HashMap later.
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r373213708
##########
File path: helix-core/src/test/resources/AzureResponse.json
##########
@@ -0,0 +1,104 @@
+{
+ "compute": {
+ "azEnvironment": "AzurePublicCloud",
+ "customData": "",
+ "location": "southcentralus",
+ "name": "ei-lid-vmss-kafka_1",
Review comment:
+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] jiajunwang commented on a change in pull request #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r376125205
##########
File path: helix-core/pom.xml
##########
@@ -159,6 +159,11 @@ under the License.
<artifactId>metrics-core</artifactId>
<version>3.2.3</version>
</dependency>
+ <dependency>
+ <groupId>org.apache.httpcomponents</groupId>
+ <artifactId>httpclient</artifactId>
+ <version>4.5.8</version>
Review comment:
nit, can you have a try to remove the corresponding entry in the helix-rest pom? I think that would be unnecessary with this.
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r373214217
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureCloudInstanceInformationProcessor.java
##########
@@ -19,38 +19,111 @@
* under the License.
*/
+import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
+import org.apache.helix.HelixCloudProperty;
+import org.apache.helix.HelixException;
import org.apache.helix.api.cloud.CloudInstanceInformationProcessor;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.util.EntityUtils;
+import org.codehaus.jackson.JsonNode;
+import org.codehaus.jackson.map.ObjectMapper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+public class AzureCloudInstanceInformationProcessor
+ implements CloudInstanceInformationProcessor<String> {
+ private CloseableHttpClient _closeableHttpClient;
Review comment:
final for both of them?
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r376143799
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureCloudInstanceInformationProcessor.java
##########
@@ -19,38 +19,135 @@
* under the License.
*/
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
import java.util.ArrayList;
import java.util.List;
+import javax.net.ssl.SSLException;
+import org.apache.helix.HelixCloudProperty;
+import org.apache.helix.HelixException;
import org.apache.helix.api.cloud.CloudInstanceInformationProcessor;
+import org.apache.http.client.HttpRequestRetryHandler;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.protocol.HttpContext;
+import org.apache.http.util.EntityUtils;
+import org.codehaus.jackson.JsonNode;
+import org.codehaus.jackson.map.ObjectMapper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+public class AzureCloudInstanceInformationProcessor
+ implements CloudInstanceInformationProcessor<String> {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(AzureCloudInstanceInformationProcessor.class);
+ private final CloseableHttpClient _closeableHttpClient;
+ private final HelixCloudProperty _helixCloudProperty;
+ private final String COMPUTE = "compute";
+ private final String INSTANCE_NAME = "vmId";
+ private final String DOMAIN = "platformFaultDomain";
+ private final String INSTANCE_SET_NAME = "vmScaleSetName";
-public class AzureCloudInstanceInformationProcessor implements CloudInstanceInformationProcessor<String> {
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty) {
+ _helixCloudProperty = helixCloudProperty;
- public AzureCloudInstanceInformationProcessor() {
+ RequestConfig requestConifg = RequestConfig.custom()
+ .setConnectionRequestTimeout((int) helixCloudProperty.getCloudRequestTimeout())
+ .setConnectTimeout((int) helixCloudProperty.getCloudConnectionTimeout()).build();
+
+ HttpRequestRetryHandler httpRequestRetryHandler =
+ (IOException exception, int executionCount, HttpContext context) -> {
+ LOG.warn("Execution count: " + executionCount + ".", exception);
+ return !(executionCount >= helixCloudProperty.getCloudMaxRetry()
+ || exception instanceof InterruptedIOException
+ || exception instanceof UnknownHostException || exception instanceof SSLException);
+ };
+
+ _closeableHttpClient = HttpClients.custom().setDefaultRequestConfig(requestConifg)
+ .setRetryHandler(httpRequestRetryHandler).build();
}
/**
- * fetch the raw Azure cloud instance information
+ * This constructor is for unit test purpose only.
+ * User could provide helixCloudProperty and a mocked http client to test the functionality of
+ * this class.
+ */
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty,
+ CloseableHttpClient closeableHttpClient) {
+ _helixCloudProperty = helixCloudProperty;
+ _closeableHttpClient = closeableHttpClient;
+ }
+
+ /**
+ * Fetch raw Azure cloud instance information based on the urls provided
* @return raw Azure cloud instance information
*/
@Override
public List<String> fetchCloudInstanceInformation() {
List<String> response = new ArrayList<>();
- //TODO: implement the fetching logic
+ for (String url : _helixCloudProperty.getCloudInfoSources()) {
+ response.add(getAzureCloudInformationFromUrl(url));
+ }
return response;
}
+ /**
+ * Query Azure Instance Metadata Service to get the instance(VM) information
+ * @return raw Azure cloud instance information
+ */
+ private String getAzureCloudInformationFromUrl(String url) {
+ HttpGet httpGet = new HttpGet(url);
+ httpGet.setHeader("Metadata", "true");
+
+ try {
+ CloseableHttpResponse response = _closeableHttpClient.execute(httpGet);
+ if (response == null || response.getStatusLine().getStatusCode() != 200) {
+ String errorMsg = String.format(
+ "Failed to get an HTTP Response for the request. Response: {}. Status code: {}",
+ (response == null ? "NULL" : response.getStatusLine().getReasonPhrase()),
+ response.getStatusLine().getStatusCode());
+ throw new HelixException(errorMsg);
+ }
+ String responseString = EntityUtils.toString(response.getEntity());
+ LOG.info("VM instance information query result: {}", responseString);
+ return responseString;
+ } catch (IOException e) {
+ throw new HelixException(
+ String.format("Failed to get Azure cloud instance information from url {}", url), e);
+ }
+ }
+
/**
* Parse raw Azure cloud instance information.
* @return required azure cloud instance information
*/
@Override
public AzureCloudInstanceInformation parseCloudInstanceInformation(List<String> responses) {
AzureCloudInstanceInformation azureCloudInstanceInformation = null;
- //TODO: implement the parsing logic
+ for (String response : responses) {
Review comment:
If there are multiple responses, they should come from different sources, e.g. some may come from local disk, and they should not have common fields. If they do, we will need to define priority among these sources.
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r373212632
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureHttpUtil.java
##########
@@ -0,0 +1,45 @@
+package org.apache.helix.cloud.azure;
+
+import javax.net.ssl.SSLException;
+import org.apache.helix.HelixCloudProperty;
+import org.apache.http.client.HttpRequestRetryHandler;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.protocol.HttpContext;
+
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A dedicated http client for retrieving information from Azure Instance Metadata Service
+ */
+class AzureHttpUtil {
+
+ private static Logger LOG = LoggerFactory.getLogger(AzureHttpUtil.class.getName());
Review comment:
Humm... This log looks different from the others. Why not just passing the class to 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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r376122844
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureHttpUtil.java
##########
@@ -0,0 +1,45 @@
+package org.apache.helix.cloud.azure;
+
+import javax.net.ssl.SSLException;
+import org.apache.helix.HelixCloudProperty;
+import org.apache.http.client.HttpRequestRetryHandler;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.protocol.HttpContext;
+
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A dedicated http client for retrieving information from Azure Instance Metadata Service
+ */
+class AzureHttpUtil {
Review comment:
Actually I double checked and found that I can just remove the util function. The processor itself can just directly make the http call, and passing the information it needs to pass. So the future usage should be, any class that needs to make the call would directly call getHttpClient, providing whatever config they would like to pass in.
For example, in current AzureCloudInstanceInfoProcessor, we only need to pass requestConfig and retryHandler. Other class may need something like "setConnectionBackoffStrategy" etc. Users can use whatever is available in the http client parameters.
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r373215016
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureCloudInstanceInformationProcessor.java
##########
@@ -19,38 +19,111 @@
* under the License.
*/
+import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
+import org.apache.helix.HelixCloudProperty;
+import org.apache.helix.HelixException;
import org.apache.helix.api.cloud.CloudInstanceInformationProcessor;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.util.EntityUtils;
+import org.codehaus.jackson.JsonNode;
+import org.codehaus.jackson.map.ObjectMapper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+public class AzureCloudInstanceInformationProcessor
+ implements CloudInstanceInformationProcessor<String> {
+ private CloseableHttpClient _closeableHttpClient;
+ private HelixCloudProperty _helixCloudProperty;
-public class AzureCloudInstanceInformationProcessor implements CloudInstanceInformationProcessor<String> {
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty) {
+ _closeableHttpClient = AzureHttpUtil.getHttpClient(helixCloudProperty);
+ _helixCloudProperty = helixCloudProperty;
+ }
- public AzureCloudInstanceInformationProcessor() {
+ /**
+ * This constructor is for unit test purpose only.
+ * User could provide helixCloudProperty and a mocked http client to test the functionality of
+ * this class.
+ */
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty,
+ CloseableHttpClient closeableHttpClient) {
+ _helixCloudProperty = helixCloudProperty;
+ _closeableHttpClient = closeableHttpClient;
}
+ private static final Logger LOG =
+ LoggerFactory.getLogger(AzureCloudInstanceInformationProcessor.class.getName());
Review comment:
nit, according to our convention,
1. class not the name please
2. put it to the top before other fields and methods.
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r373218203
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureHttpUtil.java
##########
@@ -0,0 +1,45 @@
+package org.apache.helix.cloud.azure;
+
+import javax.net.ssl.SSLException;
+import org.apache.helix.HelixCloudProperty;
+import org.apache.http.client.HttpRequestRetryHandler;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.protocol.HttpContext;
+
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A dedicated http client for retrieving information from Azure Instance Metadata Service
+ */
+class AzureHttpUtil {
Review comment:
On general question, Yi has added one httpclient in the Helix-rest for instance health check. Can we just leverage that one?
In that case, just move the code to helix-core, or better to helix-common (I hope we have one), then leverage that client.
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r373213295
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureHttpUtil.java
##########
@@ -0,0 +1,45 @@
+package org.apache.helix.cloud.azure;
+
+import javax.net.ssl.SSLException;
+import org.apache.helix.HelixCloudProperty;
+import org.apache.http.client.HttpRequestRetryHandler;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.protocol.HttpContext;
+
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A dedicated http client for retrieving information from Azure Instance Metadata Service
+ */
+class AzureHttpUtil {
+
+ private static Logger LOG = LoggerFactory.getLogger(AzureHttpUtil.class.getName());
+
+ static CloseableHttpClient getHttpClient(HelixCloudProperty helixCloudProperty) {
Review comment:
Can we just have this one instead of creating another private method which has the same logic?
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r376122844
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureHttpUtil.java
##########
@@ -0,0 +1,45 @@
+package org.apache.helix.cloud.azure;
+
+import javax.net.ssl.SSLException;
+import org.apache.helix.HelixCloudProperty;
+import org.apache.http.client.HttpRequestRetryHandler;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.protocol.HttpContext;
+
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A dedicated http client for retrieving information from Azure Instance Metadata Service
+ */
+class AzureHttpUtil {
Review comment:
Actually I double checked and found that I can just remove the util function. The processor itself can just directly make the http call, and passing the information it needs to pass. So the future usage should be, any class that needs to make the call would directly call getHttpClient, providing whatever config they would like to pass in.
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r373243524
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureCloudInstanceInformationProcessor.java
##########
@@ -19,38 +19,111 @@
* under the License.
*/
+import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
+import org.apache.helix.HelixCloudProperty;
+import org.apache.helix.HelixException;
import org.apache.helix.api.cloud.CloudInstanceInformationProcessor;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.util.EntityUtils;
+import org.codehaus.jackson.JsonNode;
+import org.codehaus.jackson.map.ObjectMapper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+public class AzureCloudInstanceInformationProcessor
+ implements CloudInstanceInformationProcessor<String> {
+ private CloseableHttpClient _closeableHttpClient;
+ private HelixCloudProperty _helixCloudProperty;
-public class AzureCloudInstanceInformationProcessor implements CloudInstanceInformationProcessor<String> {
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty) {
+ _closeableHttpClient = AzureHttpUtil.getHttpClient(helixCloudProperty);
+ _helixCloudProperty = helixCloudProperty;
+ }
- public AzureCloudInstanceInformationProcessor() {
+ /**
+ * This constructor is for unit test purpose only.
+ * User could provide helixCloudProperty and a mocked http client to test the functionality of
+ * this class.
+ */
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty,
Review comment:
I tried, compilation breaks due to testng not in the package. Meanwhile, I'd like to keep the consistency with the current tests. Let me know if you feel protected is a must.
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r373282126
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureCloudInstanceInformationProcessor.java
##########
@@ -19,38 +19,110 @@
* under the License.
*/
+import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
+import org.apache.helix.HelixCloudProperty;
+import org.apache.helix.HelixException;
import org.apache.helix.api.cloud.CloudInstanceInformationProcessor;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.util.EntityUtils;
+import org.codehaus.jackson.JsonNode;
+import org.codehaus.jackson.map.ObjectMapper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+public class AzureCloudInstanceInformationProcessor
+ implements CloudInstanceInformationProcessor<String> {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(AzureCloudInstanceInformationProcessor.class);
+ private final CloseableHttpClient _closeableHttpClient;
+ private final HelixCloudProperty _helixCloudProperty;
-public class AzureCloudInstanceInformationProcessor implements CloudInstanceInformationProcessor<String> {
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty) {
+ _closeableHttpClient = AzureHttpUtil.getHttpClient(helixCloudProperty);
+ _helixCloudProperty = helixCloudProperty;
+ }
- public AzureCloudInstanceInformationProcessor() {
+ /**
+ * This constructor is for unit test purpose only.
+ * User could provide helixCloudProperty and a mocked http client to test the functionality of
+ * this class.
+ */
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty,
+ CloseableHttpClient closeableHttpClient) {
+ _helixCloudProperty = helixCloudProperty;
+ _closeableHttpClient = closeableHttpClient;
}
/**
- * fetch the raw Azure cloud instance information
+ * Fetch raw Azure cloud instance information based on the urls provided
* @return raw Azure cloud instance information
*/
@Override
public List<String> fetchCloudInstanceInformation() {
List<String> response = new ArrayList<>();
- //TODO: implement the fetching logic
+ for (String url : _helixCloudProperty.getCloudInfoSources()) {
+ response.add(getAzureCloudInformationFromUrl(url));
+ }
return response;
}
+ /**
+ * Query Azure Instance Metadata Service to get the instance(VM) information
+ * @return raw Azure cloud instance information
+ */
+ private String getAzureCloudInformationFromUrl(String url) {
+ HttpGet httpGet = new HttpGet(url);
+ httpGet.setHeader("Metadata", "true");
+
+ try {
+ CloseableHttpResponse response = _closeableHttpClient.execute(httpGet);
+ if (response == null || response.getStatusLine().getStatusCode() != 200) {
+ String errorMsg = String.format(
+ "Failed to get an HTTP Response for the request. Response: {}. Status code: {}",
+ (response == null ? "NULL" : response.getStatusLine().getReasonPhrase()),
+ response.getStatusLine().getStatusCode());
+ throw new HelixException(errorMsg);
+ }
+ String responseString = EntityUtils.toString(response.getEntity());
+ LOG.info("VM instance information query result: {}", responseString);
+ return responseString;
+ } catch (IOException e) {
+ throw new HelixException(
+ String.format("Failed to get Azure cloud instance information from url {}", url), e);
+ }
+ }
+
/**
* Parse raw Azure cloud instance information.
* @return required azure cloud instance information
*/
@Override
public AzureCloudInstanceInformation parseCloudInstanceInformation(List<String> responses) {
AzureCloudInstanceInformation azureCloudInstanceInformation = null;
- //TODO: implement the parsing logic
+ for (String response : responses) {
+ ObjectMapper mapper = new ObjectMapper();
+ try {
+ JsonNode jsonNode = mapper.readTree(response);
+ JsonNode computeNode = jsonNode.path("compute");
+ if (!computeNode.isMissingNode()) {
+ String vmName = computeNode.path("vmId").getTextValue();
+ String platformFaultDomain = computeNode.path("platformFaultDomain").getTextValue();
+ String vmssName = computeNode.path("vmScaleSetName").getValueAsText();
Review comment:
If it's not inside a VMSS, the field will be empty. Originally VMSS is for validation purpose. Based on our current plan, it's more likely we will not use this field. However, I prefer to keep it here just in case we can still use it for validation.
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r373215475
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureCloudInstanceInformationProcessor.java
##########
@@ -19,38 +19,111 @@
* under the License.
*/
+import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
+import org.apache.helix.HelixCloudProperty;
+import org.apache.helix.HelixException;
import org.apache.helix.api.cloud.CloudInstanceInformationProcessor;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.util.EntityUtils;
+import org.codehaus.jackson.JsonNode;
+import org.codehaus.jackson.map.ObjectMapper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+public class AzureCloudInstanceInformationProcessor
+ implements CloudInstanceInformationProcessor<String> {
+ private CloseableHttpClient _closeableHttpClient;
+ private HelixCloudProperty _helixCloudProperty;
-public class AzureCloudInstanceInformationProcessor implements CloudInstanceInformationProcessor<String> {
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty) {
+ _closeableHttpClient = AzureHttpUtil.getHttpClient(helixCloudProperty);
+ _helixCloudProperty = helixCloudProperty;
+ }
- public AzureCloudInstanceInformationProcessor() {
+ /**
+ * This constructor is for unit test purpose only.
+ * User could provide helixCloudProperty and a mocked http client to test the functionality of
+ * this class.
+ */
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty,
+ CloseableHttpClient closeableHttpClient) {
+ _helixCloudProperty = helixCloudProperty;
+ _closeableHttpClient = closeableHttpClient;
}
+ private static final Logger LOG =
+ LoggerFactory.getLogger(AzureCloudInstanceInformationProcessor.class.getName());
+
/**
- * fetch the raw Azure cloud instance information
+ * Fetch raw Azure cloud instance information based on the urls provided
* @return raw Azure cloud instance information
*/
@Override
public List<String> fetchCloudInstanceInformation() {
List<String> response = new ArrayList<>();
- //TODO: implement the fetching logic
+ for (String url : _helixCloudProperty.getCloudInfoSources()) {
+ response.add(getAzureCloudInformationFromUrl(url));
+ }
return response;
}
+ /**
+ * Query Azure Instance Metadata Service to get the instance(VM) information
+ * @return raw Azure cloud instance information
+ */
+ public String getAzureCloudInformationFromUrl(String url) {
Review comment:
private ?
----------------------------------------------------------------
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] i3wangyi commented on a change in pull request #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
i3wangyi commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r376092609
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureHttpUtil.java
##########
@@ -0,0 +1,45 @@
+package org.apache.helix.cloud.azure;
+
+import javax.net.ssl.SSLException;
+import org.apache.helix.HelixCloudProperty;
+import org.apache.http.client.HttpRequestRetryHandler;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.protocol.HttpContext;
+
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A dedicated http client for retrieving information from Azure Instance Metadata Service
+ */
+class AzureHttpUtil {
Review comment:
Yes. The current CustomRestClient has one instance of `HttpClient`, a parent class of the `CloseableHttpClient` the method tries to return. However, I see the AzureHttpUtil is in helix-core while that factory is only visible in helix-rest only (because previously there's no need for helix-core to perform HTTP request).
Essentially, the factory pattern is similar and we can extract the core part of it to helix-core and leave the difference in the separate module. For example, in helix-core, create one HttpClientFactory which accepts a set of general parameters (timeout, ssl, retry..) without respecting HelixCloudProperty; And use it in rest as well as rest of places in helix-core
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r373215286
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureCloudInstanceInformationProcessor.java
##########
@@ -19,38 +19,111 @@
* under the License.
*/
+import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
+import org.apache.helix.HelixCloudProperty;
+import org.apache.helix.HelixException;
import org.apache.helix.api.cloud.CloudInstanceInformationProcessor;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.util.EntityUtils;
+import org.codehaus.jackson.JsonNode;
+import org.codehaus.jackson.map.ObjectMapper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+public class AzureCloudInstanceInformationProcessor
+ implements CloudInstanceInformationProcessor<String> {
+ private CloseableHttpClient _closeableHttpClient;
+ private HelixCloudProperty _helixCloudProperty;
-public class AzureCloudInstanceInformationProcessor implements CloudInstanceInformationProcessor<String> {
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty) {
+ _closeableHttpClient = AzureHttpUtil.getHttpClient(helixCloudProperty);
+ _helixCloudProperty = helixCloudProperty;
+ }
- public AzureCloudInstanceInformationProcessor() {
+ /**
+ * This constructor is for unit test purpose only.
+ * User could provide helixCloudProperty and a mocked http client to test the functionality of
+ * this class.
+ */
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty,
+ CloseableHttpClient closeableHttpClient) {
+ _helixCloudProperty = helixCloudProperty;
+ _closeableHttpClient = closeableHttpClient;
}
+ private static final Logger LOG =
+ LoggerFactory.getLogger(AzureCloudInstanceInformationProcessor.class.getName());
+
/**
- * fetch the raw Azure cloud instance information
+ * Fetch raw Azure cloud instance information based on the urls provided
* @return raw Azure cloud instance information
*/
@Override
public List<String> fetchCloudInstanceInformation() {
List<String> response = new ArrayList<>();
- //TODO: implement the fetching logic
+ for (String url : _helixCloudProperty.getCloudInfoSources()) {
+ response.add(getAzureCloudInformationFromUrl(url));
Review comment:
Shall we handle partial failure or not?
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r376109544
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureHttpUtil.java
##########
@@ -0,0 +1,45 @@
+package org.apache.helix.cloud.azure;
+
+import javax.net.ssl.SSLException;
+import org.apache.helix.HelixCloudProperty;
+import org.apache.http.client.HttpRequestRetryHandler;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.protocol.HttpContext;
+
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A dedicated http client for retrieving information from Azure Instance Metadata Service
+ */
+class AzureHttpUtil {
Review comment:
It is OK if we move the current client class to the helix-core now. Since there is real usage of 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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r373241501
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureHttpUtil.java
##########
@@ -0,0 +1,45 @@
+package org.apache.helix.cloud.azure;
+
+import javax.net.ssl.SSLException;
+import org.apache.helix.HelixCloudProperty;
+import org.apache.http.client.HttpRequestRetryHandler;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.protocol.HttpContext;
+
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A dedicated http client for retrieving information from Azure Instance Metadata Service
+ */
+class AzureHttpUtil {
Review comment:
I've checked the current custom rest client. It's not easy to adapt it to our use case, specifically for those user defined parameter input, such as max retry, timeout. Also the separate http client I have is easier to mock in the unit test.
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r376127133
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureCloudInstanceInformation.java
##########
@@ -42,7 +42,8 @@ public String get(String key) {
}
public static class Builder {
- private Map<String, String> _cloudInstanceInfoMap = null;
+
+ private final Map<String, String> _cloudInstanceInfoMap = new HashMap<>();
public AzureCloudInstanceInformation build() {
return new AzureCloudInstanceInformation(_cloudInstanceInfoMap);
Review comment:
Safer to clone construct a new map so the newly built instances do not share anything.
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r373232713
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureCloudInstanceInformationProcessor.java
##########
@@ -19,38 +19,111 @@
* under the License.
*/
+import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
+import org.apache.helix.HelixCloudProperty;
+import org.apache.helix.HelixException;
import org.apache.helix.api.cloud.CloudInstanceInformationProcessor;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.util.EntityUtils;
+import org.codehaus.jackson.JsonNode;
+import org.codehaus.jackson.map.ObjectMapper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+public class AzureCloudInstanceInformationProcessor
+ implements CloudInstanceInformationProcessor<String> {
+ private CloseableHttpClient _closeableHttpClient;
+ private HelixCloudProperty _helixCloudProperty;
-public class AzureCloudInstanceInformationProcessor implements CloudInstanceInformationProcessor<String> {
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty) {
+ _closeableHttpClient = AzureHttpUtil.getHttpClient(helixCloudProperty);
+ _helixCloudProperty = helixCloudProperty;
+ }
- public AzureCloudInstanceInformationProcessor() {
+ /**
+ * This constructor is for unit test purpose only.
+ * User could provide helixCloudProperty and a mocked http client to test the functionality of
+ * this class.
+ */
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty,
+ CloseableHttpClient closeableHttpClient) {
+ _helixCloudProperty = helixCloudProperty;
+ _closeableHttpClient = closeableHttpClient;
}
+ private static final Logger LOG =
+ LoggerFactory.getLogger(AzureCloudInstanceInformationProcessor.class.getName());
+
/**
- * fetch the raw Azure cloud instance information
+ * Fetch raw Azure cloud instance information based on the urls provided
* @return raw Azure cloud instance information
*/
@Override
public List<String> fetchCloudInstanceInformation() {
List<String> response = new ArrayList<>();
- //TODO: implement the fetching logic
+ for (String url : _helixCloudProperty.getCloudInfoSources()) {
+ response.add(getAzureCloudInformationFromUrl(url));
Review comment:
Cloud information is considered as one entity, if any part of it is missing/failed, we would consider it as incomplete, and fail the operation. As we cannot guarantee the fields we need later is already populated, if we do not fail here, we will need to fail in the participant manager.
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r370305488
##########
File path: helix-core/src/test/resources/AzureResponse.json
##########
@@ -0,0 +1,104 @@
+{
+ "compute": {
+ "azEnvironment": "AzurePublicCloud",
+ "customData": "",
+ "location": "southcentralus",
+ "name": "ei-lid-vmss-kafka_1",
+ "offer": "",
+ "osType": "Linux",
+ "placementGroupId": "81e605b2-a807-48ee-a84a-63c76a9c9543",
+ "plan": {
+ "name": "",
+ "product": "",
+ "publisher": ""
+ },
+ "platformFaultDomain": "2",
+ "platformUpdateDomain": "2",
+ "provider": "Microsoft.Compute",
+ "publicKeys": [],
+ "publisher": "",
+ "resourceGroupName": "scus-lpsazureei1-app-rg",
+ "resourceId": "/subscriptions/c9a251d8-1272-4c0f-8055-8271bbc1d677/resourceGroups/scus-lpsazureei1-app-rg/providers/Microsoft.Compute/virtualMachines/ei-lid-vmss-kafka_2",
+ "sku": "",
+ "storageProfile": {
+ "dataDisks": [],
+ "imageReference": {
+ "id": "/subscriptions/7dd5a659-67c4-441c-ac0b-d48b7a029668/resourceGroups/scus-infra-app-rg/providers/Microsoft.Compute/galleries/pieimagerepo/images/FastCOP4/versions/190924.1.1",
+ "offer": "",
+ "publisher": "",
+ "sku": "",
+ "version": ""
+ },
+ "osDisk": {
+ "caching": "ReadWrite",
+ "createOption": "FromImage",
+ "diskSizeGB": "32",
+ "encryptionSettings": {
+ "enabled": "false"
+ },
+ "image": {
+ "uri": ""
+ },
+ "managedDisk": {
+ "id": "/subscriptions/c9a251d8-1272-4c0f-8055-8271bbc1d677/resourceGroups/scus-lpsazureei1-app-rg/providers/Microsoft.Compute/disks/ei-lid-vmss-kafka_ei-lid-vmss-kafka_2_OsDisk_1_124c3534b8e848e296ec22b24d44c027",
+ "storageAccountType": "Standard_LRS"
+ },
+ "name": "ei-lid-vmss-kafka_ei-lid-vmss-kafka_2_OsDisk_1_124c3534b8e848e296ec22b24d44c027",
Review comment:
Same as here.
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r373213824
##########
File path: helix-core/src/test/resources/AzureResponse.json
##########
@@ -0,0 +1,104 @@
+{
+ "compute": {
+ "azEnvironment": "AzurePublicCloud",
+ "customData": "",
+ "location": "southcentralus",
+ "name": "ei-lid-vmss-kafka_1",
+ "offer": "",
+ "osType": "Linux",
+ "placementGroupId": "81e605b2-a807-48ee-a84a-63c76a9c9543",
+ "plan": {
+ "name": "",
+ "product": "",
+ "publisher": ""
+ },
+ "platformFaultDomain": "2",
+ "platformUpdateDomain": "2",
+ "provider": "Microsoft.Compute",
+ "publicKeys": [],
+ "publisher": "",
+ "resourceGroupName": "scus-lpsazureei1-app-rg",
+ "resourceId": "/subscriptions/c9a251d8-1272-4c0f-8055-8271bbc1d677/resourceGroups/scus-lpsazureei1-app-rg/providers/Microsoft.Compute/virtualMachines/ei-lid-vmss-kafka_2",
+ "sku": "",
+ "storageProfile": {
+ "dataDisks": [],
+ "imageReference": {
+ "id": "/subscriptions/7dd5a659-67c4-441c-ac0b-d48b7a029668/resourceGroups/scus-infra-app-rg/providers/Microsoft.Compute/galleries/pieimagerepo/images/FastCOP4/versions/190924.1.1",
+ "offer": "",
+ "publisher": "",
+ "sku": "",
+ "version": ""
+ },
+ "osDisk": {
+ "caching": "ReadWrite",
+ "createOption": "FromImage",
+ "diskSizeGB": "32",
+ "encryptionSettings": {
+ "enabled": "false"
+ },
+ "image": {
+ "uri": ""
+ },
+ "managedDisk": {
+ "id": "/subscriptions/c9a251d8-1272-4c0f-8055-8271bbc1d677/resourceGroups/scus-lpsazureei1-app-rg/providers/Microsoft.Compute/disks/ei-lid-vmss-kafka_ei-lid-vmss-kafka_2_OsDisk_1_124c3534b8e848e296ec22b24d44c027",
+ "storageAccountType": "Standard_LRS"
+ },
+ "name": "ei-lid-vmss-kafka_ei-lid-vmss-kafka_2_OsDisk_1_124c3534b8e848e296ec22b24d44c027",
Review comment:
+1 again : )
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r373248266
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureCloudInstanceInformationProcessor.java
##########
@@ -19,38 +19,110 @@
* under the License.
*/
+import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
+import org.apache.helix.HelixCloudProperty;
+import org.apache.helix.HelixException;
import org.apache.helix.api.cloud.CloudInstanceInformationProcessor;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.util.EntityUtils;
+import org.codehaus.jackson.JsonNode;
+import org.codehaus.jackson.map.ObjectMapper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+public class AzureCloudInstanceInformationProcessor
+ implements CloudInstanceInformationProcessor<String> {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(AzureCloudInstanceInformationProcessor.class);
+ private final CloseableHttpClient _closeableHttpClient;
+ private final HelixCloudProperty _helixCloudProperty;
-public class AzureCloudInstanceInformationProcessor implements CloudInstanceInformationProcessor<String> {
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty) {
+ _closeableHttpClient = AzureHttpUtil.getHttpClient(helixCloudProperty);
+ _helixCloudProperty = helixCloudProperty;
+ }
- public AzureCloudInstanceInformationProcessor() {
+ /**
+ * This constructor is for unit test purpose only.
+ * User could provide helixCloudProperty and a mocked http client to test the functionality of
+ * this class.
+ */
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty,
+ CloseableHttpClient closeableHttpClient) {
+ _helixCloudProperty = helixCloudProperty;
+ _closeableHttpClient = closeableHttpClient;
}
/**
- * fetch the raw Azure cloud instance information
+ * Fetch raw Azure cloud instance information based on the urls provided
* @return raw Azure cloud instance information
*/
@Override
public List<String> fetchCloudInstanceInformation() {
List<String> response = new ArrayList<>();
- //TODO: implement the fetching logic
+ for (String url : _helixCloudProperty.getCloudInfoSources()) {
+ response.add(getAzureCloudInformationFromUrl(url));
+ }
return response;
}
+ /**
+ * Query Azure Instance Metadata Service to get the instance(VM) information
+ * @return raw Azure cloud instance information
+ */
+ private String getAzureCloudInformationFromUrl(String url) {
+ HttpGet httpGet = new HttpGet(url);
+ httpGet.setHeader("Metadata", "true");
+
+ try {
+ CloseableHttpResponse response = _closeableHttpClient.execute(httpGet);
+ if (response == null || response.getStatusLine().getStatusCode() != 200) {
+ String errorMsg = String.format(
+ "Failed to get an HTTP Response for the request. Response: {}. Status code: {}",
+ (response == null ? "NULL" : response.getStatusLine().getReasonPhrase()),
+ response.getStatusLine().getStatusCode());
+ throw new HelixException(errorMsg);
+ }
+ String responseString = EntityUtils.toString(response.getEntity());
+ LOG.info("VM instance information query result: {}", responseString);
+ return responseString;
+ } catch (IOException e) {
+ throw new HelixException(
+ String.format("Failed to get Azure cloud instance information from url {}", url), e);
+ }
+ }
+
/**
* Parse raw Azure cloud instance information.
* @return required azure cloud instance information
*/
@Override
public AzureCloudInstanceInformation parseCloudInstanceInformation(List<String> responses) {
AzureCloudInstanceInformation azureCloudInstanceInformation = null;
- //TODO: implement the parsing logic
+ for (String response : responses) {
+ ObjectMapper mapper = new ObjectMapper();
+ try {
+ JsonNode jsonNode = mapper.readTree(response);
+ JsonNode computeNode = jsonNode.path("compute");
+ if (!computeNode.isMissingNode()) {
+ String vmName = computeNode.path("vmId").getTextValue();
+ String platformFaultDomain = computeNode.path("platformFaultDomain").getTextValue();
+ String vmssName = computeNode.path("vmScaleSetName").getValueAsText();
Review comment:
Is VMSS name necessary? What if this instance joined from container?
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r373211253
##########
File path: helix-core/src/main/java/org/apache/helix/api/cloud/CloudInstanceInformationProcessor.java
##########
@@ -20,6 +20,7 @@
*/
import java.util.List;
+import org.apache.helix.HelixCloudProperty;
Review comment:
Any usage in this file?
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r376128547
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureCloudInstanceInformationProcessor.java
##########
@@ -19,38 +19,135 @@
* under the License.
*/
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
import java.util.ArrayList;
import java.util.List;
+import javax.net.ssl.SSLException;
+import org.apache.helix.HelixCloudProperty;
+import org.apache.helix.HelixException;
import org.apache.helix.api.cloud.CloudInstanceInformationProcessor;
+import org.apache.http.client.HttpRequestRetryHandler;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.protocol.HttpContext;
+import org.apache.http.util.EntityUtils;
+import org.codehaus.jackson.JsonNode;
+import org.codehaus.jackson.map.ObjectMapper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+public class AzureCloudInstanceInformationProcessor
+ implements CloudInstanceInformationProcessor<String> {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(AzureCloudInstanceInformationProcessor.class);
+ private final CloseableHttpClient _closeableHttpClient;
+ private final HelixCloudProperty _helixCloudProperty;
+ private final String COMPUTE = "compute";
+ private final String INSTANCE_NAME = "vmId";
+ private final String DOMAIN = "platformFaultDomain";
+ private final String INSTANCE_SET_NAME = "vmScaleSetName";
-public class AzureCloudInstanceInformationProcessor implements CloudInstanceInformationProcessor<String> {
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty) {
+ _helixCloudProperty = helixCloudProperty;
- public AzureCloudInstanceInformationProcessor() {
+ RequestConfig requestConifg = RequestConfig.custom()
+ .setConnectionRequestTimeout((int) helixCloudProperty.getCloudRequestTimeout())
+ .setConnectTimeout((int) helixCloudProperty.getCloudConnectionTimeout()).build();
+
+ HttpRequestRetryHandler httpRequestRetryHandler =
+ (IOException exception, int executionCount, HttpContext context) -> {
+ LOG.warn("Execution count: " + executionCount + ".", exception);
+ return !(executionCount >= helixCloudProperty.getCloudMaxRetry()
+ || exception instanceof InterruptedIOException
+ || exception instanceof UnknownHostException || exception instanceof SSLException);
+ };
+
+ _closeableHttpClient = HttpClients.custom().setDefaultRequestConfig(requestConifg)
+ .setRetryHandler(httpRequestRetryHandler).build();
}
/**
- * fetch the raw Azure cloud instance information
+ * This constructor is for unit test purpose only.
+ * User could provide helixCloudProperty and a mocked http client to test the functionality of
+ * this class.
+ */
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty,
+ CloseableHttpClient closeableHttpClient) {
+ _helixCloudProperty = helixCloudProperty;
+ _closeableHttpClient = closeableHttpClient;
+ }
+
+ /**
+ * Fetch raw Azure cloud instance information based on the urls provided
* @return raw Azure cloud instance information
*/
@Override
public List<String> fetchCloudInstanceInformation() {
List<String> response = new ArrayList<>();
- //TODO: implement the fetching logic
+ for (String url : _helixCloudProperty.getCloudInfoSources()) {
+ response.add(getAzureCloudInformationFromUrl(url));
+ }
return response;
}
+ /**
+ * Query Azure Instance Metadata Service to get the instance(VM) information
+ * @return raw Azure cloud instance information
+ */
+ private String getAzureCloudInformationFromUrl(String url) {
+ HttpGet httpGet = new HttpGet(url);
+ httpGet.setHeader("Metadata", "true");
+
+ try {
+ CloseableHttpResponse response = _closeableHttpClient.execute(httpGet);
+ if (response == null || response.getStatusLine().getStatusCode() != 200) {
+ String errorMsg = String.format(
+ "Failed to get an HTTP Response for the request. Response: {}. Status code: {}",
+ (response == null ? "NULL" : response.getStatusLine().getReasonPhrase()),
+ response.getStatusLine().getStatusCode());
+ throw new HelixException(errorMsg);
+ }
+ String responseString = EntityUtils.toString(response.getEntity());
+ LOG.info("VM instance information query result: {}", responseString);
+ return responseString;
+ } catch (IOException e) {
+ throw new HelixException(
+ String.format("Failed to get Azure cloud instance information from url {}", url), e);
+ }
+ }
+
/**
* Parse raw Azure cloud instance information.
* @return required azure cloud instance information
*/
@Override
public AzureCloudInstanceInformation parseCloudInstanceInformation(List<String> responses) {
AzureCloudInstanceInformation azureCloudInstanceInformation = null;
- //TODO: implement the parsing logic
+ for (String response : responses) {
Review comment:
It confused me. If there are multiple responses, which information you return? It seems that the last one will override the previous ones.
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r376128813
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureCloudInstanceInformationProcessor.java
##########
@@ -19,38 +19,135 @@
* under the License.
*/
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
import java.util.ArrayList;
import java.util.List;
+import javax.net.ssl.SSLException;
+import org.apache.helix.HelixCloudProperty;
+import org.apache.helix.HelixException;
import org.apache.helix.api.cloud.CloudInstanceInformationProcessor;
+import org.apache.http.client.HttpRequestRetryHandler;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.protocol.HttpContext;
+import org.apache.http.util.EntityUtils;
+import org.codehaus.jackson.JsonNode;
+import org.codehaus.jackson.map.ObjectMapper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+public class AzureCloudInstanceInformationProcessor
+ implements CloudInstanceInformationProcessor<String> {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(AzureCloudInstanceInformationProcessor.class);
+ private final CloseableHttpClient _closeableHttpClient;
+ private final HelixCloudProperty _helixCloudProperty;
+ private final String COMPUTE = "compute";
+ private final String INSTANCE_NAME = "vmId";
+ private final String DOMAIN = "platformFaultDomain";
+ private final String INSTANCE_SET_NAME = "vmScaleSetName";
-public class AzureCloudInstanceInformationProcessor implements CloudInstanceInformationProcessor<String> {
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty) {
+ _helixCloudProperty = helixCloudProperty;
- public AzureCloudInstanceInformationProcessor() {
+ RequestConfig requestConifg = RequestConfig.custom()
+ .setConnectionRequestTimeout((int) helixCloudProperty.getCloudRequestTimeout())
+ .setConnectTimeout((int) helixCloudProperty.getCloudConnectionTimeout()).build();
+
+ HttpRequestRetryHandler httpRequestRetryHandler =
+ (IOException exception, int executionCount, HttpContext context) -> {
+ LOG.warn("Execution count: " + executionCount + ".", exception);
+ return !(executionCount >= helixCloudProperty.getCloudMaxRetry()
+ || exception instanceof InterruptedIOException
+ || exception instanceof UnknownHostException || exception instanceof SSLException);
+ };
+
+ _closeableHttpClient = HttpClients.custom().setDefaultRequestConfig(requestConifg)
+ .setRetryHandler(httpRequestRetryHandler).build();
}
/**
- * fetch the raw Azure cloud instance information
+ * This constructor is for unit test purpose only.
+ * User could provide helixCloudProperty and a mocked http client to test the functionality of
+ * this class.
+ */
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty,
+ CloseableHttpClient closeableHttpClient) {
+ _helixCloudProperty = helixCloudProperty;
+ _closeableHttpClient = closeableHttpClient;
+ }
+
+ /**
+ * Fetch raw Azure cloud instance information based on the urls provided
* @return raw Azure cloud instance information
*/
@Override
public List<String> fetchCloudInstanceInformation() {
List<String> response = new ArrayList<>();
Review comment:
nit, responseList or responses
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r373214601
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureCloudInstanceInformationProcessor.java
##########
@@ -19,38 +19,111 @@
* under the License.
*/
+import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
+import org.apache.helix.HelixCloudProperty;
+import org.apache.helix.HelixException;
import org.apache.helix.api.cloud.CloudInstanceInformationProcessor;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.util.EntityUtils;
+import org.codehaus.jackson.JsonNode;
+import org.codehaus.jackson.map.ObjectMapper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+public class AzureCloudInstanceInformationProcessor
+ implements CloudInstanceInformationProcessor<String> {
+ private CloseableHttpClient _closeableHttpClient;
+ private HelixCloudProperty _helixCloudProperty;
-public class AzureCloudInstanceInformationProcessor implements CloudInstanceInformationProcessor<String> {
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty) {
+ _closeableHttpClient = AzureHttpUtil.getHttpClient(helixCloudProperty);
+ _helixCloudProperty = helixCloudProperty;
+ }
- public AzureCloudInstanceInformationProcessor() {
+ /**
+ * This constructor is for unit test purpose only.
+ * User could provide helixCloudProperty and a mocked http client to test the functionality of
+ * this class.
+ */
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty,
Review comment:
Try to put the test in the same package. So this method can be protected.
And the public method should call this one instead of duplicating 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] jiajunwang commented on a change in pull request #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r376127671
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureCloudInstanceInformationProcessor.java
##########
@@ -19,38 +19,135 @@
* under the License.
*/
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
import java.util.ArrayList;
import java.util.List;
+import javax.net.ssl.SSLException;
+import org.apache.helix.HelixCloudProperty;
+import org.apache.helix.HelixException;
import org.apache.helix.api.cloud.CloudInstanceInformationProcessor;
+import org.apache.http.client.HttpRequestRetryHandler;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.protocol.HttpContext;
+import org.apache.http.util.EntityUtils;
+import org.codehaus.jackson.JsonNode;
+import org.codehaus.jackson.map.ObjectMapper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+public class AzureCloudInstanceInformationProcessor
+ implements CloudInstanceInformationProcessor<String> {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(AzureCloudInstanceInformationProcessor.class);
+ private final CloseableHttpClient _closeableHttpClient;
+ private final HelixCloudProperty _helixCloudProperty;
+ private final String COMPUTE = "compute";
+ private final String INSTANCE_NAME = "vmId";
+ private final String DOMAIN = "platformFaultDomain";
+ private final String INSTANCE_SET_NAME = "vmScaleSetName";
-public class AzureCloudInstanceInformationProcessor implements CloudInstanceInformationProcessor<String> {
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty) {
+ _helixCloudProperty = helixCloudProperty;
- public AzureCloudInstanceInformationProcessor() {
+ RequestConfig requestConifg = RequestConfig.custom()
+ .setConnectionRequestTimeout((int) helixCloudProperty.getCloudRequestTimeout())
+ .setConnectTimeout((int) helixCloudProperty.getCloudConnectionTimeout()).build();
+
+ HttpRequestRetryHandler httpRequestRetryHandler =
+ (IOException exception, int executionCount, HttpContext context) -> {
+ LOG.warn("Execution count: " + executionCount + ".", exception);
+ return !(executionCount >= helixCloudProperty.getCloudMaxRetry()
+ || exception instanceof InterruptedIOException
+ || exception instanceof UnknownHostException || exception instanceof SSLException);
+ };
+
+ _closeableHttpClient = HttpClients.custom().setDefaultRequestConfig(requestConifg)
Review comment:
As we discussed, let's add a TODO here for generalizing all the httpclients usage.
----------------------------------------------------------------
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 #698: Implement Azure cloud
instance information processor
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on issue #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#issuecomment-583647908
This PR is ready to be merged, approved by @dasahcc and @alirezazamani
Final Commit Message:
Implement Azure cloud instance information processor. The processor performs the functions of fetching and parsing instance information from Azure cloud environment. The endpoint that the processor queries is a globally standard url, called Azure Instance Metadata Service.
----------------------------------------------------------------
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 merged pull request #698: Implement Azure cloud
instance information processor
Posted by GitBox <gi...@apache.org>.
jiajunwang merged pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r375533889
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureHttpUtil.java
##########
@@ -0,0 +1,45 @@
+package org.apache.helix.cloud.azure;
+
+import javax.net.ssl.SSLException;
+import org.apache.helix.HelixCloudProperty;
+import org.apache.http.client.HttpRequestRetryHandler;
+import org.apache.http.client.config.RequestConfig;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.protocol.HttpContext;
+
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A dedicated http client for retrieving information from Azure Instance Metadata Service
+ */
+class AzureHttpUtil {
Review comment:
httpclient should be a fundamental component. If we have 2 of them in our codebase, it would be confusing and harder for us to maintain.
How difficult will it be to merge 2 use cases? Please feel free to refactor if necessary.
@i3wangyi Can you please comment as well?
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r373247382
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureCloudInstanceInformationProcessor.java
##########
@@ -19,38 +19,110 @@
* under the License.
*/
+import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
+import org.apache.helix.HelixCloudProperty;
+import org.apache.helix.HelixException;
import org.apache.helix.api.cloud.CloudInstanceInformationProcessor;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.util.EntityUtils;
+import org.codehaus.jackson.JsonNode;
+import org.codehaus.jackson.map.ObjectMapper;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+public class AzureCloudInstanceInformationProcessor
+ implements CloudInstanceInformationProcessor<String> {
+ private static final Logger LOG =
+ LoggerFactory.getLogger(AzureCloudInstanceInformationProcessor.class);
+ private final CloseableHttpClient _closeableHttpClient;
+ private final HelixCloudProperty _helixCloudProperty;
-public class AzureCloudInstanceInformationProcessor implements CloudInstanceInformationProcessor<String> {
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty) {
+ _closeableHttpClient = AzureHttpUtil.getHttpClient(helixCloudProperty);
+ _helixCloudProperty = helixCloudProperty;
+ }
- public AzureCloudInstanceInformationProcessor() {
+ /**
+ * This constructor is for unit test purpose only.
+ * User could provide helixCloudProperty and a mocked http client to test the functionality of
+ * this class.
+ */
+ public AzureCloudInstanceInformationProcessor(HelixCloudProperty helixCloudProperty,
+ CloseableHttpClient closeableHttpClient) {
+ _helixCloudProperty = helixCloudProperty;
+ _closeableHttpClient = closeableHttpClient;
}
/**
- * fetch the raw Azure cloud instance information
+ * Fetch raw Azure cloud instance information based on the urls provided
* @return raw Azure cloud instance information
*/
@Override
public List<String> fetchCloudInstanceInformation() {
List<String> response = new ArrayList<>();
- //TODO: implement the fetching logic
+ for (String url : _helixCloudProperty.getCloudInfoSources()) {
+ response.add(getAzureCloudInformationFromUrl(url));
+ }
return response;
}
+ /**
+ * Query Azure Instance Metadata Service to get the instance(VM) information
+ * @return raw Azure cloud instance information
+ */
+ private String getAzureCloudInformationFromUrl(String url) {
+ HttpGet httpGet = new HttpGet(url);
+ httpGet.setHeader("Metadata", "true");
+
+ try {
+ CloseableHttpResponse response = _closeableHttpClient.execute(httpGet);
+ if (response == null || response.getStatusLine().getStatusCode() != 200) {
+ String errorMsg = String.format(
+ "Failed to get an HTTP Response for the request. Response: {}. Status code: {}",
+ (response == null ? "NULL" : response.getStatusLine().getReasonPhrase()),
+ response.getStatusLine().getStatusCode());
+ throw new HelixException(errorMsg);
+ }
+ String responseString = EntityUtils.toString(response.getEntity());
+ LOG.info("VM instance information query result: {}", responseString);
+ return responseString;
+ } catch (IOException e) {
+ throw new HelixException(
+ String.format("Failed to get Azure cloud instance information from url {}", url), e);
+ }
+ }
+
/**
* Parse raw Azure cloud instance information.
* @return required azure cloud instance information
*/
@Override
public AzureCloudInstanceInformation parseCloudInstanceInformation(List<String> responses) {
AzureCloudInstanceInformation azureCloudInstanceInformation = null;
- //TODO: implement the parsing logic
+ for (String response : responses) {
+ ObjectMapper mapper = new ObjectMapper();
+ try {
+ JsonNode jsonNode = mapper.readTree(response);
+ JsonNode computeNode = jsonNode.path("compute");
Review comment:
Let's make these as variables instead of hard 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] jiajunwang commented on a change in pull request #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r373211720
##########
File path: helix-core/src/main/java/org/apache/helix/cloud/azure/AzureCloudInstanceInformation.java
##########
@@ -42,7 +42,13 @@ public String get(String key) {
}
public static class Builder {
- private Map<String, String> _cloudInstanceInfoMap = null;
+ /**
+ * Default constructor
+ */
+ public Builder() {
Review comment:
nit, I think this is optional if you don't have any other constructor?
----------------------------------------------------------------
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 #698:
Implement Azure cloud instance information processor
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #698: Implement Azure cloud instance information processor
URL: https://github.com/apache/helix/pull/698#discussion_r370305161
##########
File path: helix-core/src/test/resources/AzureResponse.json
##########
@@ -0,0 +1,104 @@
+{
+ "compute": {
+ "azEnvironment": "AzurePublicCloud",
+ "customData": "",
+ "location": "southcentralus",
+ "name": "ei-lid-vmss-kafka_1",
Review comment:
I would prefer to use other names such as "Test-Helix" in our tests.
----------------------------------------------------------------
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