From feb67dedeb05a2400537596ea86341031d4cb9fa Mon Sep 17 00:00:00 2001 From: Chaitanya Deepthi Date: Sat, 19 Oct 2024 13:06:55 -0700 Subject: [PATCH 1/4] Test failure fix --- .../apache/pinot/controller/ControllerStarterStatelessTest.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pinot-controller/src/test/java/org/apache/pinot/controller/ControllerStarterStatelessTest.java b/pinot-controller/src/test/java/org/apache/pinot/controller/ControllerStarterStatelessTest.java index afa3e17d2fe..b43c5599961 100644 --- a/pinot-controller/src/test/java/org/apache/pinot/controller/ControllerStarterStatelessTest.java +++ b/pinot-controller/src/test/java/org/apache/pinot/controller/ControllerStarterStatelessTest.java @@ -40,6 +40,8 @@ public class ControllerStarterStatelessTest extends ControllerTest { @Override protected void overrideControllerConf(Map properties) { + //To not test dynamic config env variables through this class. Host is not dynamic for this class config + properties.remove("dynamic.env.config"); properties.putAll(_configOverride); } From 9f2550a85393d64b5b10dde5087f5af29c96178e Mon Sep 17 00:00:00 2001 From: Chaitanya Deepthi Date: Sat, 19 Oct 2024 14:14:32 -0700 Subject: [PATCH 2/4] Handle error logs for false case scenarios --- .../apache/pinot/spi/env/PinotConfiguration.java | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/pinot-spi/src/main/java/org/apache/pinot/spi/env/PinotConfiguration.java b/pinot-spi/src/main/java/org/apache/pinot/spi/env/PinotConfiguration.java index 02fa4349022..22230b2c8cc 100644 --- a/pinot-spi/src/main/java/org/apache/pinot/spi/env/PinotConfiguration.java +++ b/pinot-spi/src/main/java/org/apache/pinot/spi/env/PinotConfiguration.java @@ -175,10 +175,16 @@ public static List applyDynamicEnvConfig(List conf return configurations.stream().peek(configuration -> { if (!configuration.getBoolean(TEMPLATED_KEY, false)) { for (String dynamicEnvConfigVarName : configuration.getStringArray(ENV_DYNAMIC_CONFIG_KEY)) { - configuration.setProperty(dynamicEnvConfigVarName, - environmentVariables.get(configuration.getString(dynamicEnvConfigVarName))); - LOGGER.info("The environment variable is set to the property {} through dynamic configs", - dynamicEnvConfigVarName); + String envVariable = configuration.getString(dynamicEnvConfigVarName); + assert envVariable != null; + Object envVarValue = environmentVariables.get(envVariable); + if (envVarValue != null) { + configuration.setProperty(dynamicEnvConfigVarName, envVarValue); + LOGGER.info("The environment variable is set to the property {} through dynamic configs", + dynamicEnvConfigVarName); + } else { + LOGGER.error("The environment variable {} is not found", envVariable); + } } configuration.setProperty(TEMPLATED_KEY, true); } From e34761319e6d0723faa848e06912f8300233608e Mon Sep 17 00:00:00 2001 From: Chaitanya Deepthi Date: Sat, 19 Oct 2024 16:33:09 -0700 Subject: [PATCH 3/4] Add a new class to test the dynamic env variables separate --- .../ControllerStarterDynamicEnvTest.java | 191 ++++++++++++++++++ .../ControllerStarterStatelessTest.java | 2 - .../controller/helix/ControllerTest.java | 16 +- .../PinotControllerModeStatelessTest.java | 10 +- ...ontrollerLeaderLocatorIntegrationTest.java | 2 +- .../pinot/spi/env/PinotConfiguration.java | 1 - 6 files changed, 199 insertions(+), 23 deletions(-) create mode 100644 pinot-controller/src/test/java/org/apache/pinot/controller/ControllerStarterDynamicEnvTest.java diff --git a/pinot-controller/src/test/java/org/apache/pinot/controller/ControllerStarterDynamicEnvTest.java b/pinot-controller/src/test/java/org/apache/pinot/controller/ControllerStarterDynamicEnvTest.java new file mode 100644 index 00000000000..67f94e21f67 --- /dev/null +++ b/pinot-controller/src/test/java/org/apache/pinot/controller/ControllerStarterDynamicEnvTest.java @@ -0,0 +1,191 @@ +/** + * 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.controller; + +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import org.apache.helix.ConfigAccessor; +import org.apache.helix.model.ClusterConfig; +import org.apache.helix.model.HelixConfigScope; +import org.apache.helix.model.InstanceConfig; +import org.apache.helix.model.builder.HelixConfigScopeBuilder; +import org.apache.pinot.common.utils.helix.HelixHelper; +import org.apache.pinot.controller.helix.ControllerTest; +import org.apache.pinot.spi.env.PinotConfiguration; +import org.apache.pinot.spi.utils.CommonConstants; +import org.apache.pinot.spi.utils.NetUtils; +import org.apache.pinot.spi.utils.builder.ControllerRequestURLBuilder; +import org.testng.annotations.Test; + +import static org.apache.pinot.controller.ControllerConf.CONTROLLER_HOST; +import static org.apache.pinot.controller.ControllerConf.CONTROLLER_PORT; +import static org.apache.pinot.spi.utils.CommonConstants.Controller.CONFIG_OF_INSTANCE_ID; +import static org.apache.pinot.spi.utils.CommonConstants.Helix.CONTROLLER_INSTANCE; +import static org.testng.Assert.*; + + +/** + * This class tests env variables when starting controller from configs + */ +public class ControllerStarterDynamicEnvTest extends ControllerTest { + private final Map _configOverride = new HashMap<>(); + + @Override + protected void overrideControllerConf(Map properties) { + //To not test dynamic config env variables through this class. Host is not dynamic for this class config + properties.putAll(_configOverride); + } + + @Test + public void testNoVariable() + throws Exception { + _configOverride.clear(); + _configOverride.put(CONTROLLER_HOST, "myHost"); + _configOverride.put(CONFIG_OF_INSTANCE_ID, "Controller_myInstance"); + _configOverride.put(CONTROLLER_PORT, 1234); + + startZk(); + this.startController(); + + String instanceId = _controllerStarter.getInstanceId(); + assertEquals(instanceId, "Controller_myInstance"); + InstanceConfig instanceConfig = HelixHelper.getInstanceConfig(_helixManager, instanceId); + assertEquals(instanceConfig.getInstanceName(), instanceId); + assertEquals(instanceConfig.getHostName(), "myHost"); + assertEquals(instanceConfig.getPort(), "1234"); + assertEquals(instanceConfig.getTags(), Collections.singleton(CONTROLLER_INSTANCE)); + + stopController(); + stopZk(); + } + + @Test + public void testOneVariable() + throws Exception { + _configOverride.clear(); + _configOverride.put("dynamic.env.config", "controller.host"); + _configOverride.put(CONTROLLER_HOST, "HOST"); + _configOverride.put(CONFIG_OF_INSTANCE_ID, "Controller_myInstance"); + _configOverride.put(CONTROLLER_PORT, 1234); + + startZk(); + this.startController(); + + String instanceId = _controllerStarter.getInstanceId(); + assertEquals(instanceId, "Controller_myInstance"); + InstanceConfig instanceConfig = HelixHelper.getInstanceConfig(_helixManager, instanceId); + assertEquals(instanceConfig.getInstanceName(), instanceId); + assertEquals(instanceConfig.getHostName(), "myHost"); + assertEquals(instanceConfig.getPort(), "1234"); + assertEquals(instanceConfig.getTags(), Collections.singleton(CONTROLLER_INSTANCE)); + + stopController(); + stopZk(); + } + + @Test + public void testMultipleVariables() + throws Exception { + _configOverride.clear(); + _configOverride.put("dynamic.env.config", "controller.host,controller.port"); + _configOverride.put(CONTROLLER_HOST, "HOST"); + _configOverride.put(CONTROLLER_PORT, "PORT"); + _configOverride.put(CONFIG_OF_INSTANCE_ID, "Controller_myInstance"); + + startZk(); + this.startController(); + + String instanceId = _controllerStarter.getInstanceId(); + assertEquals(instanceId, "Controller_myInstance"); + InstanceConfig instanceConfig = HelixHelper.getInstanceConfig(_helixManager, instanceId); + assertEquals(instanceConfig.getInstanceName(), instanceId); + assertEquals(instanceConfig.getHostName(), "myHost"); + assertEquals(instanceConfig.getPort(), "1234"); + assertEquals(instanceConfig.getTags(), Collections.singleton(CONTROLLER_INSTANCE)); + + stopController(); + stopZk(); + } + + @Override + public Map getDefaultControllerConfiguration() { + Map properties = new HashMap<>(); + properties.put(ControllerConf.ZK_STR, getZkUrl()); + properties.put(ControllerConf.HELIX_CLUSTER_NAME, getHelixClusterName()); + properties.put("dynamic.env.config", "controller.host,controller.port"); + properties.put(CONTROLLER_HOST, "HOST"); + properties.put(CONTROLLER_PORT, "PORT"); + + int controllerPort = NetUtils.findOpenPort(_nextControllerPort); + properties.put(ControllerConf.CONTROLLER_PORT, controllerPort); + if (_controllerPort == 0) { + _controllerPort = controllerPort; + } + _nextControllerPort = controllerPort + 1; + properties.put(ControllerConf.DATA_DIR, DEFAULT_DATA_DIR); + properties.put(ControllerConf.LOCAL_TEMP_DIR, DEFAULT_LOCAL_TEMP_DIR); + // Enable groovy on the controller + properties.put(ControllerConf.DISABLE_GROOVY, false); + properties.put(CommonConstants.CONFIG_OF_TIMEZONE, "UTC"); + overrideControllerConf(properties); + return properties; + } + + @Override + public void startController(Map properties) + throws Exception { + Map envVariables = new HashMap<>(); + envVariables.put("HOST", "myHost"); + envVariables.put("PORT", "1234"); + _controllerStarter = createControllerStarter(); + _controllerStarter.init(new PinotConfiguration(properties, envVariables)); + _controllerStarter.start(); + _controllerConfig = _controllerStarter.getConfig(); + _controllerBaseApiUrl = _controllerConfig.generateVipUrl(); + _controllerRequestURLBuilder = ControllerRequestURLBuilder.baseUrl(_controllerBaseApiUrl); + _controllerDataDir = _controllerConfig.getDataDir(); + _helixResourceManager = _controllerStarter.getHelixResourceManager(); + _helixManager = _controllerStarter.getHelixControllerManager(); + _helixDataAccessor = _helixManager.getHelixDataAccessor(); + ConfigAccessor configAccessor = _helixManager.getConfigAccessor(); + // HelixResourceManager is null in Helix only mode, while HelixManager is null in Pinot only mode. + HelixConfigScope scope = + new HelixConfigScopeBuilder(HelixConfigScope.ConfigScopeProperty.CLUSTER).forCluster(getHelixClusterName()) + .build(); + switch (_controllerStarter.getControllerMode()) { + case DUAL: + case PINOT_ONLY: + _helixAdmin = _helixResourceManager.getHelixAdmin(); + _propertyStore = _helixResourceManager.getPropertyStore(); + // TODO: Enable periodic rebalance per 10 seconds as a temporary work-around for the Helix issue: + // https://github.com/apache/helix/issues/331 and https://github.com/apache/helix/issues/2309. + // Remove this after Helix fixing the issue. + configAccessor.set(scope, ClusterConfig.ClusterConfigProperty.REBALANCE_TIMER_PERIOD.name(), "10000"); + break; + case HELIX_ONLY: + _helixAdmin = _helixManager.getClusterManagmentTool(); + _propertyStore = _helixManager.getHelixPropertyStore(); + break; + default: + break; + } + assertEquals(System.getProperty("user.timezone"), "UTC"); + } +} diff --git a/pinot-controller/src/test/java/org/apache/pinot/controller/ControllerStarterStatelessTest.java b/pinot-controller/src/test/java/org/apache/pinot/controller/ControllerStarterStatelessTest.java index b43c5599961..afa3e17d2fe 100644 --- a/pinot-controller/src/test/java/org/apache/pinot/controller/ControllerStarterStatelessTest.java +++ b/pinot-controller/src/test/java/org/apache/pinot/controller/ControllerStarterStatelessTest.java @@ -40,8 +40,6 @@ public class ControllerStarterStatelessTest extends ControllerTest { @Override protected void overrideControllerConf(Map properties) { - //To not test dynamic config env variables through this class. Host is not dynamic for this class config - properties.remove("dynamic.env.config"); properties.putAll(_configOverride); } diff --git a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java index 9f7d7692cdf..a95c75ca118 100644 --- a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java +++ b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java @@ -88,7 +88,6 @@ public class ControllerTest { public static final String LOCAL_HOST = "localhost"; - public static final String LOCAL_HOST_CONTROLLER = "LC"; public static final String DEFAULT_DATA_DIR = new File(FileUtils.getTempDirectoryPath(), "test-controller-data-dir" + System.currentTimeMillis()).getAbsolutePath(); public static final String DEFAULT_LOCAL_TEMP_DIR = new File(FileUtils.getTempDirectoryPath(), @@ -137,7 +136,6 @@ public class ControllerTest { protected HelixDataAccessor _helixDataAccessor; protected HelixAdmin _helixAdmin; protected ZkHelixPropertyStore _propertyStore; - protected Map _envVariables = new HashMap<>(); /** * Acquire the {@link ControllerTest} default instance that can be shared across different test cases. @@ -166,11 +164,6 @@ public static HttpClient getHttpClient() { return _httpClient; } - public Map getEnvVariables() { - _envVariables.putIfAbsent("LC", "localhost"); - return _envVariables; - } - /** * ControllerRequestClient is lazy evaluated, static object, only instantiate when first use. * @@ -216,8 +209,6 @@ public Map getDefaultControllerConfiguration() { Map properties = new HashMap<>(); properties.put(ControllerConf.ZK_STR, getZkUrl()); properties.put(ControllerConf.HELIX_CLUSTER_NAME, getHelixClusterName()); - properties.put(ControllerConf.CONTROLLER_HOST, LOCAL_HOST_CONTROLLER); - properties.put("dynamic.env.config", ControllerConf.CONTROLLER_HOST); int controllerPort = NetUtils.findOpenPort(_nextControllerPort); properties.put(ControllerConf.CONTROLLER_PORT, controllerPort); if (_controllerPort == 0) { @@ -255,10 +246,8 @@ public void startController(Map properties) throws Exception { assertNull(_controllerStarter, "Controller is already started"); assertTrue(_controllerPort > 0, "Controller port is not assigned"); - Map envVariables = new HashMap<>(); - envVariables.put("LC", "localhost"); _controllerStarter = createControllerStarter(); - _controllerStarter.init(new PinotConfiguration(properties, envVariables)); + _controllerStarter.init(new PinotConfiguration(properties)); _controllerStarter.start(); _controllerConfig = _controllerStarter.getConfig(); _controllerBaseApiUrl = _controllerConfig.generateVipUrl(); @@ -978,8 +967,7 @@ public ControllerConf getControllerConfig() { /** * Do not override this method as the configuration is shared across all default TestNG group. */ - public final Map getSharedControllerConfiguration() - throws IOException { + public final Map getSharedControllerConfiguration() { Map properties = getDefaultControllerConfiguration(); // TODO: move these test specific configs into respective test classes. diff --git a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/PinotControllerModeStatelessTest.java b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/PinotControllerModeStatelessTest.java index f6f2e5fb4e3..47a1121481e 100644 --- a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/PinotControllerModeStatelessTest.java +++ b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/PinotControllerModeStatelessTest.java @@ -121,7 +121,7 @@ public void testDualModeController() properties = getDefaultControllerConfiguration(); properties.put(ControllerConf.CONTROLLER_MODE, ControllerConf.ControllerMode.DUAL); ControllerStarter secondDualModeController = new ControllerStarter(); - secondDualModeController.init(new PinotConfiguration(properties, getEnvVariables())); + secondDualModeController.init(new PinotConfiguration(properties)); secondDualModeController.start(); TestUtils .waitForCondition(aVoid -> secondDualModeController.getHelixResourceManager().getHelixZkManager().isConnected(), @@ -169,7 +169,7 @@ public void testDualModeController() properties.put(ControllerConf.CONTROLLER_MODE, ControllerConf.ControllerMode.DUAL); ControllerStarter thirdDualModeController = new ControllerStarter(); - thirdDualModeController.init(new PinotConfiguration(properties, getEnvVariables())); + thirdDualModeController.init(new PinotConfiguration(properties)); thirdDualModeController.start(); PinotHelixResourceManager pinotHelixResourceManager = thirdDualModeController.getHelixResourceManager(); _helixManager = pinotHelixResourceManager.getHelixZkManager(); @@ -210,7 +210,7 @@ public void testPinotOnlyController() properties.put(ControllerConf.CONTROLLER_MODE, ControllerConf.ControllerMode.PINOT_ONLY); ControllerStarter firstPinotOnlyController = new ControllerStarter(); - firstPinotOnlyController.init(new PinotConfiguration(properties, getEnvVariables())); + firstPinotOnlyController.init(new PinotConfiguration(properties)); // Starting Pinot-only controller without Helix controller should fail try { @@ -225,7 +225,7 @@ public void testPinotOnlyController() properties.put(ControllerConf.CONTROLLER_MODE, ControllerConf.ControllerMode.HELIX_ONLY); ControllerStarter helixOnlyController = new ControllerStarter(); - helixOnlyController.init(new PinotConfiguration(properties, getEnvVariables())); + helixOnlyController.init(new PinotConfiguration(properties)); helixOnlyController.start(); HelixManager helixControllerManager = helixOnlyController.getHelixControllerManager(); HelixAdmin helixAdmin = helixControllerManager.getClusterManagmentTool(); @@ -252,7 +252,7 @@ public void testPinotOnlyController() properties.put(ControllerConf.CONTROLLER_MODE, ControllerConf.ControllerMode.PINOT_ONLY); ControllerStarter secondPinotOnlyController = new ControllerStarter(); - secondPinotOnlyController.init(new PinotConfiguration(properties, getEnvVariables())); + secondPinotOnlyController.init(new PinotConfiguration(properties)); secondPinotOnlyController.start(); TestUtils.waitForCondition( aVoid -> secondPinotOnlyController.getHelixResourceManager().getHelixZkManager().isConnected(), TIMEOUT_IN_MS, diff --git a/pinot-integration-tests/src/test/java/org/apache/pinot/server/realtime/ControllerLeaderLocatorIntegrationTest.java b/pinot-integration-tests/src/test/java/org/apache/pinot/server/realtime/ControllerLeaderLocatorIntegrationTest.java index 54071fa3361..ecb587f38f8 100644 --- a/pinot-integration-tests/src/test/java/org/apache/pinot/server/realtime/ControllerLeaderLocatorIntegrationTest.java +++ b/pinot-integration-tests/src/test/java/org/apache/pinot/server/realtime/ControllerLeaderLocatorIntegrationTest.java @@ -71,7 +71,7 @@ public void testControllerLeaderLocator() // Use custom instance id properties.put(Controller.CONFIG_OF_INSTANCE_ID, "Controller_myInstance"); ControllerStarter secondControllerStarter = new ControllerStarter(); - secondControllerStarter.init(new PinotConfiguration(properties, getEnvVariables())); + secondControllerStarter.init(new PinotConfiguration(properties)); secondControllerStarter.start(); TestUtils diff --git a/pinot-spi/src/main/java/org/apache/pinot/spi/env/PinotConfiguration.java b/pinot-spi/src/main/java/org/apache/pinot/spi/env/PinotConfiguration.java index 22230b2c8cc..d3ba835dc4a 100644 --- a/pinot-spi/src/main/java/org/apache/pinot/spi/env/PinotConfiguration.java +++ b/pinot-spi/src/main/java/org/apache/pinot/spi/env/PinotConfiguration.java @@ -176,7 +176,6 @@ public static List applyDynamicEnvConfig(List conf if (!configuration.getBoolean(TEMPLATED_KEY, false)) { for (String dynamicEnvConfigVarName : configuration.getStringArray(ENV_DYNAMIC_CONFIG_KEY)) { String envVariable = configuration.getString(dynamicEnvConfigVarName); - assert envVariable != null; Object envVarValue = environmentVariables.get(envVariable); if (envVarValue != null) { configuration.setProperty(dynamicEnvConfigVarName, envVarValue); From 93c98b13840af5823bbc09688620a9479cf977b1 Mon Sep 17 00:00:00 2001 From: Chaitanya Deepthi Date: Sat, 19 Oct 2024 17:14:16 -0700 Subject: [PATCH 4/4] Fix tests from controller --- .../java/org/apache/pinot/controller/helix/ControllerTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java index a95c75ca118..fa5b8a08000 100644 --- a/pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java +++ b/pinot-controller/src/test/java/org/apache/pinot/controller/helix/ControllerTest.java @@ -209,6 +209,7 @@ public Map getDefaultControllerConfiguration() { Map properties = new HashMap<>(); properties.put(ControllerConf.ZK_STR, getZkUrl()); properties.put(ControllerConf.HELIX_CLUSTER_NAME, getHelixClusterName()); + properties.put(ControllerConf.CONTROLLER_HOST, LOCAL_HOST); int controllerPort = NetUtils.findOpenPort(_nextControllerPort); properties.put(ControllerConf.CONTROLLER_PORT, controllerPort); if (_controllerPort == 0) {