From add71e8d23ee951ee5b16bad259f6b5d10c3ad76 Mon Sep 17 00:00:00 2001 From: shuwenwei Date: Thu, 12 Dec 2024 11:29:01 +0800 Subject: [PATCH 1/5] ignore undefined config items --- .../confignode/manager/ConfigManager.java | 31 +++++++++++++------ .../apache/iotdb/db/conf/IoTDBDescriptor.java | 2 +- .../executor/ClusterConfigTaskExecutor.java | 7 +++-- .../iotdb/db/storageengine/StorageEngine.java | 4 +++ .../commons/conf/ConfigurationFileUtils.java | 25 ++++++++++----- 5 files changed, 49 insertions(+), 20 deletions(-) diff --git a/iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/ConfigManager.java b/iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/ConfigManager.java index e83b56aaeb8a..dd178ef69696 100644 --- a/iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/ConfigManager.java +++ b/iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/ConfigManager.java @@ -1573,25 +1573,36 @@ public TSStatus clearCache() { public TSStatus setConfiguration(TSetConfigurationReq req) { TSStatus tsStatus = new TSStatus(TSStatusCode.SUCCESS_STATUS.getStatusCode()); int currentNodeId = CONF.getConfigNodeId(); - if (req.getNodeId() < 0 || currentNodeId == req.getNodeId()) { - URL url = ConfigNodeDescriptor.getPropsUrl(CommonConfig.SYSTEM_CONFIG_NAME); - if (url == null || !new File(url.getFile()).exists()) { + if (currentNodeId != req.getNodeId()) { + tsStatus = confirmLeader(); + if (tsStatus.getCode() != TSStatusCode.SUCCESS_STATUS.getStatusCode()) { return tsStatus; } - File file = new File(url.getFile()); + } + if (currentNodeId == req.getNodeId() || req.getNodeId() < 0) { + URL url = ConfigNodeDescriptor.getPropsUrl(CommonConfig.SYSTEM_CONFIG_NAME); + boolean configurationFileFound = (url != null && new File(url.getFile()).exists()); Properties properties = new Properties(); properties.putAll(req.getConfigs()); - try { - ConfigurationFileUtils.updateConfigurationFile(file, properties); - } catch (Exception e) { - return RpcUtils.getStatus(TSStatusCode.EXECUTE_STATEMENT_ERROR, e.getMessage()); + + if (configurationFileFound) { + File file = new File(url.getFile()); + try { + ConfigurationFileUtils.updateConfigurationFile(file, properties); + } catch (Exception e) { + tsStatus = RpcUtils.getStatus(TSStatusCode.EXECUTE_STATEMENT_ERROR, e.getMessage()); + } + } else { + String msg = + "Unable to find the configuration file. Some modifications are made only in memory."; + tsStatus = RpcUtils.getStatus(TSStatusCode.EXECUTE_STATEMENT_ERROR, msg); + LOGGER.warn(msg); } ConfigNodeDescriptor.getInstance().loadHotModifiedProps(properties); - if (CONF.getConfigNodeId() == req.getNodeId()) { + if (currentNodeId == req.getNodeId()) { return tsStatus; } } - tsStatus = confirmLeader(); return tsStatus.getCode() == TSStatusCode.SUCCESS_STATUS.getStatusCode() ? RpcUtils.squashResponseStatusList(nodeManager.setConfiguration(req)) : tsStatus; diff --git a/iotdb-core/datanode/src/main/java/org/apache/iotdb/db/conf/IoTDBDescriptor.java b/iotdb-core/datanode/src/main/java/org/apache/iotdb/db/conf/IoTDBDescriptor.java index b5e29e5c901a..55a96fd009c8 100644 --- a/iotdb-core/datanode/src/main/java/org/apache/iotdb/db/conf/IoTDBDescriptor.java +++ b/iotdb-core/datanode/src/main/java/org/apache/iotdb/db/conf/IoTDBDescriptor.java @@ -2022,7 +2022,7 @@ public synchronized void loadHotModifiedProps() throws QueryProcessException { try (InputStream inputStream = url.openStream()) { LOGGER.info("Start to reload config file {}", url); commonProperties.load(new InputStreamReader(inputStream, StandardCharsets.UTF_8)); - ConfigurationFileUtils.getConfigurationDefaultValue(); + ConfigurationFileUtils.loadConfigurationDefaultValueFromTemplate(); loadHotModifiedProps(commonProperties); } catch (Exception e) { LOGGER.warn("Fail to reload config file {}", url, e); diff --git a/iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/config/executor/ClusterConfigTaskExecutor.java b/iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/config/executor/ClusterConfigTaskExecutor.java index 618a9cd53dcb..f8e08f0db496 100644 --- a/iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/config/executor/ClusterConfigTaskExecutor.java +++ b/iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/execution/config/executor/ClusterConfigTaskExecutor.java @@ -1061,11 +1061,14 @@ public SettableFuture setConfiguration(TSetConfigurationReq re TSStatus tsStatus = new TSStatus(TSStatusCode.SUCCESS_STATUS.getStatusCode()); List ignoredConfigItems = - ConfigurationFileUtils.filterImmutableConfigItems(req.getConfigs()); + ConfigurationFileUtils.filterInvalidConfigItems(req.getConfigs()); TSStatus warningTsStatus = null; if (!ignoredConfigItems.isEmpty()) { warningTsStatus = new TSStatus(TSStatusCode.EXECUTE_STATEMENT_ERROR.getStatusCode()); - warningTsStatus.setMessage("ignored config items: " + ignoredConfigItems); + warningTsStatus.setMessage( + "ignored config items: " + + ignoredConfigItems + + " because they are immutable or undefined."); if (req.getConfigs().isEmpty()) { future.setException(new IoTDBException(warningTsStatus.message, warningTsStatus.code)); return future; diff --git a/iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/StorageEngine.java b/iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/StorageEngine.java index 0baf58a172d6..56470075c779 100644 --- a/iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/StorageEngine.java +++ b/iotdb-core/datanode/src/main/java/org/apache/iotdb/db/storageengine/StorageEngine.java @@ -662,6 +662,10 @@ public TSStatus setConfiguration(TSetConfigurationReq req) { URL configFileUrl = IoTDBDescriptor.getPropsUrl(CommonConfig.SYSTEM_CONFIG_NAME); if (configFileUrl == null || !(new File(configFileUrl.getFile()).exists())) { // configuration file not exist, update in mem + String msg = + "Unable to find the configuration file. Some modifications are made only in memory."; + tsStatus = RpcUtils.getStatus(TSStatusCode.EXECUTE_STATEMENT_ERROR, msg); + LOGGER.warn(msg); try { IoTDBDescriptor.getInstance().loadHotModifiedProps(properties); IoTDBDescriptor.getInstance().reloadMetricProperties(properties); diff --git a/iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/conf/ConfigurationFileUtils.java b/iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/conf/ConfigurationFileUtils.java index 0b86d5e48c63..0857b493b9a4 100644 --- a/iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/conf/ConfigurationFileUtils.java +++ b/iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/conf/ConfigurationFileUtils.java @@ -158,7 +158,7 @@ public static String readConfigFileContent(URL url) throws IOException { return readConfigLines(f); } - public static void getConfigurationDefaultValue() throws IOException { + public static void loadConfigurationDefaultValueFromTemplate() throws IOException { if (configuration2DefaultValue != null) { return; } @@ -185,6 +185,7 @@ public static void getConfigurationDefaultValue() throws IOException { } } } catch (IOException e) { + configuration2DefaultValue = null; logger.warn("Failed to read configuration template", e); throw e; } @@ -197,7 +198,7 @@ public static String getConfigurationDefaultValue(String parameterName) throws I if (configuration2DefaultValue != null) { return configuration2DefaultValue.get(parameterName); } else { - getConfigurationDefaultValue(); + loadConfigurationDefaultValueFromTemplate(); return configuration2DefaultValue.getOrDefault(parameterName, null); } } @@ -225,14 +226,24 @@ public static String readConfigurationTemplateFile() throws IOException { return content.toString(); } - public static List filterImmutableConfigItems(Map configItems) { + public static List filterInvalidConfigItems(Map configItems) { + boolean successLoadDefaultValueMap = true; + try { + loadConfigurationDefaultValueFromTemplate(); + } catch (IOException e) { + successLoadDefaultValueMap = false; + } + List ignoredConfigItems = new ArrayList<>(); - for (String ignoredKey : ignoreConfigKeys) { - if (configItems.containsKey(ignoredKey)) { - configItems.remove(ignoredKey); - ignoredConfigItems.add(ignoredKey); + for (String key : configItems.keySet()) { + if (ignoreConfigKeys.contains(key)) { + ignoredConfigItems.add(key); + } + if (successLoadDefaultValueMap && !configuration2DefaultValue.containsKey(key)) { + ignoredConfigItems.add(key); } } + ignoredConfigItems.forEach(configItems::remove); return ignoredConfigItems; } From b99237ccc8e8eba6841f31be39a62a6375c49229 Mon Sep 17 00:00:00 2001 From: shuwenwei Date: Thu, 12 Dec 2024 11:50:42 +0800 Subject: [PATCH 2/5] modify ConfigManager --- .../apache/iotdb/confignode/manager/ConfigManager.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/ConfigManager.java b/iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/ConfigManager.java index dd178ef69696..20aec73a23d0 100644 --- a/iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/ConfigManager.java +++ b/iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/manager/ConfigManager.java @@ -1603,9 +1603,11 @@ public TSStatus setConfiguration(TSetConfigurationReq req) { return tsStatus; } } - return tsStatus.getCode() == TSStatusCode.SUCCESS_STATUS.getStatusCode() - ? RpcUtils.squashResponseStatusList(nodeManager.setConfiguration(req)) - : tsStatus; + List statusListOfOtherNodes = nodeManager.setConfiguration(req); + List statusList = new ArrayList<>(statusListOfOtherNodes.size() + 1); + statusList.add(tsStatus); + statusList.addAll(statusListOfOtherNodes); + return RpcUtils.squashResponseStatusList(statusList); } @Override From 988f28b4ee8b21ad8edebb763a89151811024acd Mon Sep 17 00:00:00 2001 From: shuwenwei Date: Thu, 12 Dec 2024 12:23:22 +0800 Subject: [PATCH 3/5] modify it --- .../iotdb/db/it/IoTDBSetConfigurationIT.java | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/integration-test/src/test/java/org/apache/iotdb/db/it/IoTDBSetConfigurationIT.java b/integration-test/src/test/java/org/apache/iotdb/db/it/IoTDBSetConfigurationIT.java index 9039f94dc18a..6e92b6a1147c 100644 --- a/integration-test/src/test/java/org/apache/iotdb/db/it/IoTDBSetConfigurationIT.java +++ b/integration-test/src/test/java/org/apache/iotdb/db/it/IoTDBSetConfigurationIT.java @@ -55,6 +55,35 @@ public static void tearDown() throws Exception { EnvFactory.getEnv().cleanClusterEnvironment(); } + @Test + public void testSetConfigurationWithUndefinedConfigKey() { + try (Connection connection = EnvFactory.getEnv().getConnection(); + Statement statement = connection.createStatement()) { + executeAndExpectException(statement, "set configuration \"a\"=\"false\""); + int configNodeNum = EnvFactory.getEnv().getConfigNodeWrapperList().size(); + int dataNodeNum = EnvFactory.getEnv().getDataNodeWrapperList().size(); + + for (int i = 0; i < configNodeNum; i++) { + executeAndExpectException(statement, "set configuration \"a\"=\"false\" on " + i); + } + for (int i = 0; i < dataNodeNum; i++) { + int dnId = configNodeNum + i; + executeAndExpectException(statement, "set configuration \"a\"=\"false\" on " + dnId); + } + } catch (Exception e) { + Assert.fail(e.getMessage()); + } + } + + private void executeAndExpectException(Statement statement, String sql) { + try { + statement.execute(sql); + } catch (Exception ignored) { + return; + } + Assert.fail(); + } + @Test public void testSetConfiguration() { try (Connection connection = EnvFactory.getEnv().getConnection(); From 78d85194e4c0143d3425c7265803745593cf1122 Mon Sep 17 00:00:00 2001 From: shuwenwei Date: Thu, 12 Dec 2024 15:56:27 +0800 Subject: [PATCH 4/5] check exception msg --- .../iotdb/db/it/IoTDBSetConfigurationIT.java | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/integration-test/src/test/java/org/apache/iotdb/db/it/IoTDBSetConfigurationIT.java b/integration-test/src/test/java/org/apache/iotdb/db/it/IoTDBSetConfigurationIT.java index 6e92b6a1147c..22c8d60d4654 100644 --- a/integration-test/src/test/java/org/apache/iotdb/db/it/IoTDBSetConfigurationIT.java +++ b/integration-test/src/test/java/org/apache/iotdb/db/it/IoTDBSetConfigurationIT.java @@ -57,28 +57,35 @@ public static void tearDown() throws Exception { @Test public void testSetConfigurationWithUndefinedConfigKey() { + String expectedExceptionMsg = + "301: ignored config items: [a] because they are immutable or undefined."; try (Connection connection = EnvFactory.getEnv().getConnection(); Statement statement = connection.createStatement()) { - executeAndExpectException(statement, "set configuration \"a\"=\"false\""); + executeAndExpectException( + statement, "set configuration \"a\"=\"false\"", expectedExceptionMsg); int configNodeNum = EnvFactory.getEnv().getConfigNodeWrapperList().size(); int dataNodeNum = EnvFactory.getEnv().getDataNodeWrapperList().size(); for (int i = 0; i < configNodeNum; i++) { - executeAndExpectException(statement, "set configuration \"a\"=\"false\" on " + i); + executeAndExpectException( + statement, "set configuration \"a\"=\"false\" on " + i, expectedExceptionMsg); } for (int i = 0; i < dataNodeNum; i++) { int dnId = configNodeNum + i; - executeAndExpectException(statement, "set configuration \"a\"=\"false\" on " + dnId); + executeAndExpectException( + statement, "set configuration \"a\"=\"false\" on " + dnId, expectedExceptionMsg); } } catch (Exception e) { Assert.fail(e.getMessage()); } } - private void executeAndExpectException(Statement statement, String sql) { + private void executeAndExpectException( + Statement statement, String sql, String expectedContentInExceptionMsg) { try { statement.execute(sql); - } catch (Exception ignored) { + } catch (Exception e) { + Assert.assertTrue(e.getMessage().contains(expectedContentInExceptionMsg)); return; } Assert.fail(); From 103283868cf3aab47133b2971e87ab7905ee1b71 Mon Sep 17 00:00:00 2001 From: shuwenwei Date: Fri, 13 Dec 2024 18:12:23 +0800 Subject: [PATCH 5/5] fix it --- .../iotdb/db/it/IoTDBSetConfigurationIT.java | 43 +++++++++++++++---- 1 file changed, 35 insertions(+), 8 deletions(-) diff --git a/integration-test/src/test/java/org/apache/iotdb/db/it/IoTDBSetConfigurationIT.java b/integration-test/src/test/java/org/apache/iotdb/db/it/IoTDBSetConfigurationIT.java index 22c8d60d4654..da6db7a12a81 100644 --- a/integration-test/src/test/java/org/apache/iotdb/db/it/IoTDBSetConfigurationIT.java +++ b/integration-test/src/test/java/org/apache/iotdb/db/it/IoTDBSetConfigurationIT.java @@ -20,6 +20,7 @@ package org.apache.iotdb.db.it; import org.apache.iotdb.commons.conf.CommonConfig; +import org.apache.iotdb.commons.conf.ConfigurationFileUtils; import org.apache.iotdb.it.env.EnvFactory; import org.apache.iotdb.it.env.cluster.node.AbstractNodeWrapper; import org.apache.iotdb.it.framework.IoTDBTestRunner; @@ -40,6 +41,7 @@ import java.sql.ResultSet; import java.sql.Statement; import java.util.Arrays; +import java.util.Properties; import java.util.concurrent.TimeUnit; @RunWith(IoTDBTestRunner.class) @@ -155,24 +157,49 @@ public void testSetClusterName() throws Exception { Awaitility.await() .atMost(10, TimeUnit.SECONDS) .until(() -> !EnvFactory.getEnv().getDataNodeWrapper(0).isAlive()); + AbstractNodeWrapper datanode = EnvFactory.getEnv().getDataNodeWrapper(0); Assert.assertTrue( checkConfigFileContains(EnvFactory.getEnv().getDataNodeWrapper(0), "cluster_name=yy")); + + // Modify the config file manually because the datanode can not restart + Properties properties = new Properties(); + properties.put("cluster_name", "xx"); + ConfigurationFileUtils.updateConfigurationFile(getConfigFile(datanode), properties); + EnvFactory.getEnv().getDataNodeWrapper(0).stop(); + EnvFactory.getEnv().getDataNodeWrapper(0).start(); + // wait the datanode restart successfully (won't do any meaningful modification) + Awaitility.await() + .atMost(30, TimeUnit.SECONDS) + .pollDelay(1, TimeUnit.SECONDS) + .until( + () -> { + try (Connection connection = EnvFactory.getEnv().getConnection(); + Statement statement = connection.createStatement()) { + statement.execute("set configuration \"cluster_name\"=\"xx\" on 1"); + } catch (Exception e) { + return false; + } + return true; + }); } private static boolean checkConfigFileContains( AbstractNodeWrapper nodeWrapper, String... contents) { try { - String systemPropertiesPath = - nodeWrapper.getNodePath() - + File.separator - + "conf" - + File.separator - + CommonConfig.SYSTEM_CONFIG_NAME; - File f = new File(systemPropertiesPath); - String fileContent = new String(Files.readAllBytes(f.toPath())); + String fileContent = new String(Files.readAllBytes(getConfigFile(nodeWrapper).toPath())); return Arrays.stream(contents).allMatch(fileContent::contains); } catch (IOException ignore) { return false; } } + + private static File getConfigFile(AbstractNodeWrapper nodeWrapper) { + String systemPropertiesPath = + nodeWrapper.getNodePath() + + File.separator + + "conf" + + File.separator + + CommonConfig.SYSTEM_CONFIG_NAME; + return new File(systemPropertiesPath); + } }