mcvsubbu commented on a change in pull request #6842:
URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r628638866



##########
File path: 
pinot-plugins/pinot-environment/pinot-azure/src/main/java/org/apache/pinot/plugin/provider/AzureEnvironmentProvider.java
##########
@@ -0,0 +1,171 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.plugin.provider;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
+import java.util.HashMap;
+import java.util.Map;
+import javax.net.ssl.SSLException;
+import javax.ws.rs.WebApplicationException;
+import org.apache.commons.lang.StringUtils;
+import org.apache.http.HttpEntityEnclosingRequest;
+import org.apache.http.HttpStatus;
+import org.apache.http.StatusLine;
+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.client.protocol.HttpClientContext;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.util.EntityUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.environmentprovider.PinotEnvironmentProvider;
+
+/**
+ * Azure Environment Provider used to retrieve azure cloud specific instance 
configuration.
+ */
+public class AzureEnvironmentProvider implements PinotEnvironmentProvider {
+  protected static final String MAX_RETRY = "maxRetry";
+  protected static final String IMDS_ENDPOINT = "imdsEndpoint";
+  protected static final String CONNECTION_TIMEOUT = "connectionTimeout";
+  protected static final String REQUEST_TIMEOUT = "requestTimeout";
+  private static final String COMPUTE = "compute";
+  private static final String METADATA = "Metadata";
+  private static final String PLATFORM_FAULT_DOMAIN = "platformFaultDomain";
+  private int _maxRetry;
+  private String _imdsEndpoint;
+  private CloseableHttpClient _closeableHttpClient;
+  private PinotConfiguration _serverConfigs;
+
+  public AzureEnvironmentProvider() {
+  }
+
+  public void init(PinotConfiguration pinotConfiguration) throws 
NullPointerException, IllegalArgumentException {
+    _serverConfigs = pinotConfiguration;
+    Preconditions.checkArgument(0 < 
Integer.parseInt(_serverConfigs.getProperty(MAX_RETRY)),

Review comment:
       Please include the keyname in your error/exception message. There are 
many configuration values, and we don't want a user asking in the slack channel 
which retry we are referring to.
   I would suggest a standard error message like this: 
"AzureEnvironmentProvider: <key> should be <some expression>". And yes, please 
use the property name from the key. 

##########
File path: 
pinot-plugins/pinot-environment/pinot-azure/src/main/java/org/apache/pinot/plugin/provider/AzureEnvironmentProvider.java
##########
@@ -0,0 +1,171 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.plugin.provider;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
+import java.util.HashMap;
+import java.util.Map;
+import javax.net.ssl.SSLException;
+import javax.ws.rs.WebApplicationException;
+import org.apache.commons.lang.StringUtils;
+import org.apache.http.HttpEntityEnclosingRequest;
+import org.apache.http.HttpStatus;
+import org.apache.http.StatusLine;
+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.client.protocol.HttpClientContext;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.util.EntityUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.environmentprovider.PinotEnvironmentProvider;
+
+/**
+ * Azure Environment Provider used to retrieve azure cloud specific instance 
configuration.
+ */
+public class AzureEnvironmentProvider implements PinotEnvironmentProvider {
+  protected static final String MAX_RETRY = "maxRetry";
+  protected static final String IMDS_ENDPOINT = "imdsEndpoint";
+  protected static final String CONNECTION_TIMEOUT = "connectionTimeout";
+  protected static final String REQUEST_TIMEOUT = "requestTimeout";
+  private static final String COMPUTE = "compute";
+  private static final String METADATA = "Metadata";
+  private static final String PLATFORM_FAULT_DOMAIN = "platformFaultDomain";
+  private int _maxRetry;
+  private String _imdsEndpoint;
+  private CloseableHttpClient _closeableHttpClient;
+  private PinotConfiguration _serverConfigs;
+
+  public AzureEnvironmentProvider() {
+  }
+
+  public void init(PinotConfiguration pinotConfiguration) throws 
NullPointerException, IllegalArgumentException {
+    _serverConfigs = pinotConfiguration;
+    Preconditions.checkArgument(0 < 
Integer.parseInt(_serverConfigs.getProperty(MAX_RETRY)),
+         "maxRetry cannot be less than or equal to 0");
+    
Preconditions.checkArgument(!StringUtils.isBlank(_serverConfigs.getProperty(IMDS_ENDPOINT)),
+        "imdsEndpoint should not be null or empty");
+
+    _maxRetry = Integer.parseInt(_serverConfigs.getProperty(MAX_RETRY));
+    _imdsEndpoint = _serverConfigs.getProperty(IMDS_ENDPOINT);
+    int connectionTimeout = 
Integer.parseInt(_serverConfigs.getProperty(CONNECTION_TIMEOUT));
+    int requestTimeout = 
Integer.parseInt(_serverConfigs.getProperty(REQUEST_TIMEOUT));
+
+    final RequestConfig requestConfig = RequestConfig.custom()
+        .setConnectTimeout(connectionTimeout)
+        .setConnectionRequestTimeout(requestTimeout)
+        .build();
+
+    final HttpRequestRetryHandler httpRequestRetryHandler = (iOException, 
executionCount, httpContext) ->
+        !(executionCount >= _maxRetry
+            || iOException instanceof InterruptedIOException
+            || iOException instanceof UnknownHostException
+            || iOException instanceof SSLException
+            || HttpClientContext.adapt(httpContext).getRequest() instanceof 
HttpEntityEnclosingRequest);
+
+    _closeableHttpClient =
+        
HttpClients.custom().setDefaultRequestConfig(requestConfig).setRetryHandler(httpRequestRetryHandler).build();
+  }
+
+  // Constructor for test purposes.

Review comment:
       Please annotate as @VisibleForTesting
   

##########
File path: 
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -154,6 +160,45 @@ public HelixServerStarter(String helixClusterName, String 
zkAddress, PinotConfig
             Server.DEFAULT_CURRENT_DATA_TABLE_VERSION));
   }
 
+  /**
+   *  Invoke pinot environment provider factory's init method to register the 
environment provider &
+   *  fetch the overridden server configs for the invoked environment provider.
+   */
+  private void fetchEnvironmentSpecificConfigs() {
+    PinotConfiguration environmentProviderConfigs = _serverConf.subset(
+        Server.PREFIX_OF_CONFIG_OF_ENVIRONMENT_PROVIDER_FACTORY);
+
+    if (environmentProviderConfigs.toMap().isEmpty()) {
+      LOGGER.info("No Environment Provider Configs found.");
+      return;
+    }
+
+    PinotEnvironmentProviderFactory.init(environmentProviderConfigs);
+    String environmentProviderClassName = 
_serverConf.getProperty(Server.ENVIRONMENT_PROVIDER_CLASS_NAME);
+    if (environmentProviderClassName == null) {
+      LOGGER.info("No Class Name provided for Environment Provider.");
+      return;
+    }
+
+    // Fetch environment provider instance
+    PinotEnvironmentProvider pinotEnvironmentProvider = 
PinotEnvironmentProviderFactory.getEnvironmentProvider(
+        environmentProviderClassName.toLowerCase());
+
+    // Fetch the overridden pinot configuration containing custom configs.
+    Map<String, Object> overriddenPinotConfigs = 
pinotEnvironmentProvider.getEnvironment();
+
+    // Populate environment specific fields in the map.
+    String failureDomain = 
(String)overriddenPinotConfigs.get(PinotEnvironmentProvider.INSTANCE_FAILURE_DOMAIN);
+    if (failureDomain == null) {
+      LOGGER.info("No failure domain information found for instance: {}", 
_instanceId);

Review comment:
       Please add the class name that we used to get the fault domain info. 
That way, an operator will know which class to fix/update.

##########
File path: 
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixServerStarter.java
##########
@@ -154,6 +160,45 @@ public HelixServerStarter(String helixClusterName, String 
zkAddress, PinotConfig
             Server.DEFAULT_CURRENT_DATA_TABLE_VERSION));
   }
 
+  /**
+   *  Invoke pinot environment provider factory's init method to register the 
environment provider &
+   *  fetch the overridden server configs for the invoked environment provider.
+   */
+  private void fetchEnvironmentSpecificConfigs() {
+    PinotConfiguration environmentProviderConfigs = _serverConf.subset(
+        Server.PREFIX_OF_CONFIG_OF_ENVIRONMENT_PROVIDER_FACTORY);
+
+    if (environmentProviderConfigs.toMap().isEmpty()) {
+      LOGGER.info("No Environment Provider Configs found.");
+      return;
+    }
+
+    PinotEnvironmentProviderFactory.init(environmentProviderConfigs);
+    String environmentProviderClassName = 
_serverConf.getProperty(Server.ENVIRONMENT_PROVIDER_CLASS_NAME);
+    if (environmentProviderClassName == null) {
+      LOGGER.info("No Class Name provided for Environment Provider.");

Review comment:
       Add the property name in the log msg please

##########
File path: 
pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProvider.java
##########
@@ -0,0 +1,42 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.environmentprovider;
+
+import java.util.Map;
+import org.apache.pinot.spi.env.PinotConfiguration;
+
+/**
+ *  Environment Provider interface implemented by different cloud providers to 
customize
+ *  the base pinot configuration to add environment variables & instance 
specific configuration
+ */
+public interface PinotEnvironmentProvider {
+
+  String INSTANCE_FAILURE_DOMAIN = "pinot.environment.instance.failureDomain";

Review comment:
       This is a top level config? If so, should it go with the other configs? 
Or, is is "pinot.server.pinot.environment.instance..." (in which case the 
second pinot seems to be unnecessary)?

##########
File path: 
pinot-plugins/pinot-environment/pinot-azure/src/main/java/org/apache/pinot/plugin/provider/AzureEnvironmentProvider.java
##########
@@ -0,0 +1,171 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.plugin.provider;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Preconditions;
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.UnknownHostException;
+import java.util.HashMap;
+import java.util.Map;
+import javax.net.ssl.SSLException;
+import javax.ws.rs.WebApplicationException;
+import org.apache.commons.lang.StringUtils;
+import org.apache.http.HttpEntityEnclosingRequest;
+import org.apache.http.HttpStatus;
+import org.apache.http.StatusLine;
+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.client.protocol.HttpClientContext;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.util.EntityUtils;
+import org.apache.pinot.spi.env.PinotConfiguration;
+import org.apache.pinot.spi.environmentprovider.PinotEnvironmentProvider;
+
+/**
+ * Azure Environment Provider used to retrieve azure cloud specific instance 
configuration.
+ */
+public class AzureEnvironmentProvider implements PinotEnvironmentProvider {
+  protected static final String MAX_RETRY = "maxRetry";
+  protected static final String IMDS_ENDPOINT = "imdsEndpoint";
+  protected static final String CONNECTION_TIMEOUT = "connectionTimeout";
+  protected static final String REQUEST_TIMEOUT = "requestTimeout";
+  private static final String COMPUTE = "compute";
+  private static final String METADATA = "Metadata";
+  private static final String PLATFORM_FAULT_DOMAIN = "platformFaultDomain";
+  private int _maxRetry;
+  private String _imdsEndpoint;
+  private CloseableHttpClient _closeableHttpClient;
+  private PinotConfiguration _serverConfigs;
+
+  public AzureEnvironmentProvider() {
+  }
+
+  public void init(PinotConfiguration pinotConfiguration) throws 
NullPointerException, IllegalArgumentException {
+    _serverConfigs = pinotConfiguration;
+    Preconditions.checkArgument(0 < 
Integer.parseInt(_serverConfigs.getProperty(MAX_RETRY)),
+         "maxRetry cannot be less than or equal to 0");
+    
Preconditions.checkArgument(!StringUtils.isBlank(_serverConfigs.getProperty(IMDS_ENDPOINT)),
+        "imdsEndpoint should not be null or empty");
+
+    _maxRetry = Integer.parseInt(_serverConfigs.getProperty(MAX_RETRY));
+    _imdsEndpoint = _serverConfigs.getProperty(IMDS_ENDPOINT);
+    int connectionTimeout = 
Integer.parseInt(_serverConfigs.getProperty(CONNECTION_TIMEOUT));
+    int requestTimeout = 
Integer.parseInt(_serverConfigs.getProperty(REQUEST_TIMEOUT));
+
+    final RequestConfig requestConfig = RequestConfig.custom()
+        .setConnectTimeout(connectionTimeout)
+        .setConnectionRequestTimeout(requestTimeout)
+        .build();
+
+    final HttpRequestRetryHandler httpRequestRetryHandler = (iOException, 
executionCount, httpContext) ->
+        !(executionCount >= _maxRetry
+            || iOException instanceof InterruptedIOException
+            || iOException instanceof UnknownHostException
+            || iOException instanceof SSLException
+            || HttpClientContext.adapt(httpContext).getRequest() instanceof 
HttpEntityEnclosingRequest);
+
+    _closeableHttpClient =
+        
HttpClients.custom().setDefaultRequestConfig(requestConfig).setRetryHandler(httpRequestRetryHandler).build();
+  }
+
+  // Constructor for test purposes.
+  public AzureEnvironmentProvider(int maxRetry, String imdsEndpoint, 
CloseableHttpClient closeableHttpClient) {
+    Preconditions.checkArgument(maxRetry > 0, "maxRetry cannot be less than or 
equal to 0");

Review comment:
       We don't need pre-conditions if we don't use this ctor for anything 
other than testing, right?




-- 
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:
us...@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org

Reply via email to