mcvsubbu commented on a change in pull request #6842: URL: https://github.com/apache/incubator-pinot/pull/6842#discussion_r629786020
########## File path: pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProviderFactory.java ########## @@ -0,0 +1,82 @@ +/** + * 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 com.google.common.base.Preconditions; +import com.google.common.collect.Iterables; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import org.apache.pinot.spi.env.PinotConfiguration; +import org.apache.pinot.spi.plugin.PluginManager; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * This factory class initializes the PinotEnvironmentProvider class. + * It creates a PinotEnvironment object based on the URI found. + */ +public class PinotEnvironmentProviderFactory { + + private PinotEnvironmentProviderFactory() { + } + + private static final Logger LOGGER = LoggerFactory.getLogger(PinotEnvironmentProviderFactory.class); + private static final String CLASS = "class"; + private static final Map<String, PinotEnvironmentProvider> PINOT_ENVIRONMENT_PROVIDER_MAP = new HashMap<>(); + + public static void init(PinotConfiguration environmentProviderFactoryConfig) { + // Get environment and it's respective class + PinotConfiguration environmentConfiguration = environmentProviderFactoryConfig.subset(CLASS); + List<String> environments = environmentConfiguration.getKeys(); + + if (environments.isEmpty()) { + LOGGER.info("Did not find any environment provider classes in the configuration"); + return; + } + + String environment = Iterables.getOnlyElement(environments); + String environmentProviderClassName = environmentConfiguration.getProperty(environment); + PinotConfiguration environmentProviderConfiguration = environmentProviderFactoryConfig.subset(environment); + LOGGER.info("Got environment {}, initializing class {}", environment, environmentProviderClassName); + register(environment, environmentProviderClassName, environmentProviderConfiguration); + } + + // Utility to invoke the cloud specific environment provider. + public static PinotEnvironmentProvider getEnvironmentProvider(String environment) { + PinotEnvironmentProvider pinotEnvironmentProvider = PINOT_ENVIRONMENT_PROVIDER_MAP.get(environment); + Preconditions.checkState(pinotEnvironmentProvider != null, + "PinotEnvironmentProvider for environment: %s has not been initialized", environment); + return pinotEnvironmentProvider; + } + + private static void register( + String environment, String environmentProviderFileName, PinotConfiguration environmentProviderConfiguration) { + try { + LOGGER.info("Initializing PinotEnvironmentProvider for environment {}, classname {}", environment, environmentProviderFileName); + PinotEnvironmentProvider pinotEnvironmentProvider = PluginManager.get().createInstance(environmentProviderFileName); + pinotEnvironmentProvider.init(environmentProviderConfiguration); + PINOT_ENVIRONMENT_PROVIDER_MAP.put(environment, pinotEnvironmentProvider); Review comment: It may be a good idea to at least detect duplicate registrations and register either the first or last one (could be useful in an Integration test env where we may start more than one server, for example) ########## File path: pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProviderFactory.java ########## @@ -0,0 +1,82 @@ +/** + * 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 com.google.common.base.Preconditions; +import com.google.common.collect.Iterables; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import org.apache.pinot.spi.env.PinotConfiguration; +import org.apache.pinot.spi.plugin.PluginManager; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * This factory class initializes the PinotEnvironmentProvider class. + * It creates a PinotEnvironment object based on the URI found. + */ +public class PinotEnvironmentProviderFactory { + + private PinotEnvironmentProviderFactory() { + } + + private static final Logger LOGGER = LoggerFactory.getLogger(PinotEnvironmentProviderFactory.class); + private static final String CLASS = "class"; + private static final Map<String, PinotEnvironmentProvider> PINOT_ENVIRONMENT_PROVIDER_MAP = new HashMap<>(); Review comment: ```suggestion private static final Map<String, PinotEnvironmentProvider> ENVNAME_2_ENVPROVIDER_MAP = new HashMap<>(); ``` I am assuming the key is env name? ########## 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"; + + /** + * Customize base pinot configuration to add environment variables & instance specific configuration + * @return custom pinot configuration map + */ + Map<String, Object> getEnvironment(); Review comment: Why return a map from the Env provider? Map is how Pinot chooses to store objects into zookeeper instance config. If we return a map from here, Pinot does not know how to parse the map. Instead of this, I would declare a specific call like getFailureDomain(). Also, another call getFailureDomain(NodeName). In the first case, it returns a string with the failure domain name of the node in which the call is made. In the second case, it returns a string with the failure domain name of the specified node. ########## File path: pinot-spi/src/main/java/org/apache/pinot/spi/environmentprovider/PinotEnvironmentProviderFactory.java ########## @@ -0,0 +1,82 @@ +/** + * 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 com.google.common.base.Preconditions; +import com.google.common.collect.Iterables; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import org.apache.pinot.spi.env.PinotConfiguration; +import org.apache.pinot.spi.plugin.PluginManager; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * This factory class initializes the PinotEnvironmentProvider class. + * It creates a PinotEnvironment object based on the URI found. + */ +public class PinotEnvironmentProviderFactory { + + private PinotEnvironmentProviderFactory() { + } + + private static final Logger LOGGER = LoggerFactory.getLogger(PinotEnvironmentProviderFactory.class); + private static final String CLASS = "class"; + private static final Map<String, PinotEnvironmentProvider> PINOT_ENVIRONMENT_PROVIDER_MAP = new HashMap<>(); + + public static void init(PinotConfiguration environmentProviderFactoryConfig) { + // Get environment and it's respective class + PinotConfiguration environmentConfiguration = environmentProviderFactoryConfig.subset(CLASS); + List<String> environments = environmentConfiguration.getKeys(); + + if (environments.isEmpty()) { + LOGGER.info("Did not find any environment provider classes in the configuration"); + return; + } + + String environment = Iterables.getOnlyElement(environments); + String environmentProviderClassName = environmentConfiguration.getProperty(environment); + PinotConfiguration environmentProviderConfiguration = environmentProviderFactoryConfig.subset(environment); + LOGGER.info("Got environment {}, initializing class {}", environment, environmentProviderClassName); + register(environment, environmentProviderClassName, environmentProviderConfiguration); + } + + // Utility to invoke the cloud specific environment provider. + public static PinotEnvironmentProvider getEnvironmentProvider(String environment) { + PinotEnvironmentProvider pinotEnvironmentProvider = PINOT_ENVIRONMENT_PROVIDER_MAP.get(environment); + Preconditions.checkState(pinotEnvironmentProvider != null, + "PinotEnvironmentProvider for environment: %s has not been initialized", environment); + return pinotEnvironmentProvider; + } + + private static void register( + String environment, String environmentProviderFileName, PinotConfiguration environmentProviderConfiguration) { Review comment: ```suggestion String environment, String environmentProviderClassName, PinotConfiguration environmentProviderConfiguration) { ``` ########## 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"); + Preconditions.checkArgument(!StringUtils.isBlank(imdsEndpoint), "imdsEndpoint should not be null or empty"); + _maxRetry = maxRetry; + _imdsEndpoint = imdsEndpoint; + _closeableHttpClient = Preconditions.checkNotNull(closeableHttpClient, "Closeable Http Client cannot be null"); + } + + /** + * + * Method for constructing custom pinot configuration used by the HelixServerStarter to update + * zookeeper node with custom instance configs. + * @return custom pinot configuration map + */ + @Override + public Map<String, Object> getEnvironment() { + Map<String, Object> customPinotConfiguration = new HashMap<>(_serverConfigs.toMap()); + + // Populate failure domain information + customPinotConfiguration.put(INSTANCE_FAILURE_DOMAIN, getFailureDomain()); + + return customPinotConfiguration; + } + + // Utility used to query the azure instance metadata service (Azure IMDS) to fetch the fault/failure domain information. + @VisibleForTesting + protected final String getFailureDomain() { + final String responsePayload = getAzureInstanceMetadata(); + + // For a sample response payload, + // check https://docs.microsoft.com/en-us/azure/virtual-machines/windows/instance-metadata-service?tabs=linux + final ObjectMapper objectMapper = new ObjectMapper(); + try { + final JsonNode jsonNode = objectMapper.readTree(responsePayload); + final JsonNode computeNode = jsonNode.path(COMPUTE); + + if (computeNode.isMissingNode()) { + throw new WebApplicationException( Review comment: Why is this throwing WedApplicationException? Please change it to RuntimeException or a specific UnknownEnvException. Also, you need to decide what HelixServerStarter should do if it gets an exception from this call. Do you want to proceed to start the service without failure domain, or block the server from starting? ########## 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"); + Preconditions.checkArgument(!StringUtils.isBlank(imdsEndpoint), "imdsEndpoint should not be null or empty"); + _maxRetry = maxRetry; + _imdsEndpoint = imdsEndpoint; + _closeableHttpClient = Preconditions.checkNotNull(closeableHttpClient, "Closeable Http Client cannot be null"); + } + + /** + * + * Method for constructing custom pinot configuration used by the HelixServerStarter to update + * zookeeper node with custom instance configs. + * @return custom pinot configuration map + */ + @Override + public Map<String, Object> getEnvironment() { Review comment: See comments in the interface -- 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
