[Common][Server] Adapt and add test cases

michaelkoepf · michaelkoepf · commit e7dd2f38ea0d · 2025-04-06T21:31:40.000+02:00
diff --git a/fluss-common/src/main/java/com/alibaba/fluss/config/GlobalConfiguration.java b/fluss-common/src/main/java/com/alibaba/fluss/config/GlobalConfiguration.java
@@ -17,7 +17,6 @@
 package com.alibaba.fluss.config;
 
 import com.alibaba.fluss.annotation.Internal;
-import com.alibaba.fluss.annotation.VisibleForTesting;
 import com.alibaba.fluss.exception.IllegalConfigurationException;
 
 import org.slf4j.Logger;
@@ -50,8 +49,8 @@ public class GlobalConfiguration {
 
     private static final Logger LOG = LoggerFactory.getLogger(GlobalConfiguration.class);
 
-    @VisibleForTesting
-    public static final String[] FLUSS_CONF_FILENAME = new String[] {"server.yaml", "common.yaml"};
+    private static final String[] FLUSS_CONF_FILENAMES =
+            new String[] {"server.yaml", "common.yaml"};
 
     // --------------------------------------------------------------------------------------------
 
@@ -136,28 +135,28 @@ public static Configuration loadConfiguration(
             }
 
             // get Fluss yaml configuration files from dir
-            final File serverYamlFile = new File(confDirFile, FLUSS_CONF_FILENAME[0]);
-            final File commonYamlFile = new File(confDirFile, FLUSS_CONF_FILENAME[1]);
+            final File serverYamlFile = new File(confDirFile, FLUSS_CONF_FILENAMES[0]);
+            final File commonYamlFile = new File(confDirFile, FLUSS_CONF_FILENAMES[1]);
 
             // 1. check if old and new configuration files are mixed which is not supported
             if (serverYamlFile.exists() && commonYamlFile.exists()) {
                 throw new IllegalConfigurationException(
                         "Only one of "
-                                + FLUSS_CONF_FILENAME[0]
+                                + FLUSS_CONF_FILENAMES[0]
                                 + " and "
-                                + FLUSS_CONF_FILENAME[1]
+                                + FLUSS_CONF_FILENAMES[1]
                                 + " may be specified.");
             }
 
             // 2. backward compatability, use server.yaml
             if (serverYamlFile.exists()) {
-                yamlConfigFiles.add(new File(confDirFile, FLUSS_CONF_FILENAME[0]));
+                yamlConfigFiles.add(new File(confDirFile, FLUSS_CONF_FILENAMES[0]));
             }
 
             // 3. latest configuration setup: load common.yaml and additionally specified, dedicated
             // configuration files
             if (commonYamlFile.exists()) {
-                yamlConfigFiles.add(new File(confDirFile, FLUSS_CONF_FILENAME[1]));
+                yamlConfigFiles.add(new File(confDirFile, FLUSS_CONF_FILENAMES[1]));
 
                 for (String additionalDefaultFile : additionalDefaultFiles) {
                     yamlConfigFiles.add(new File(confDirFile, additionalDefaultFile));
diff --git a/fluss-common/src/test/java/com/alibaba/fluss/config/GlobalConfigurationTest.java b/fluss-common/src/test/java/com/alibaba/fluss/config/GlobalConfigurationTest.java
@@ -0,0 +1,81 @@
+/*
+ *  Copyright (c) 2025 Alibaba Group Holding Ltd.
+ *
+ *  Licensed 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 com.alibaba.fluss.config;
+
+import com.alibaba.fluss.exception.IllegalConfigurationException;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Collections;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+/** Tests for {@link com.alibaba.fluss.config.GlobalConfiguration}. */
+public class GlobalConfigurationTest {
+
+    @Test
+    void testLoadConfigurationWithoutAdditionalFiles(@TempDir Path tempFolder) throws Exception {
+        String confDir = tempFolder.toAbsolutePath().toString();
+
+        // backward compatability for old common configuration file
+        Path serverYaml = tempFolder.resolve("server.yaml");
+        Files.write(serverYaml, Collections.singleton("coordinator.host: localhost"));
+
+        Configuration configuration = GlobalConfiguration.loadConfiguration(confDir, null);
+        assertThat(configuration.get(ConfigOptions.COORDINATOR_HOST)).isEqualTo("localhost");
+
+        // backward compatability for old common configuration file + precedence for dynamic
+        // properties
+        Configuration dynamicConfig = new Configuration();
+        dynamicConfig.set(ConfigOptions.COORDINATOR_HOST, "example.com");
+        configuration = GlobalConfiguration.loadConfiguration(confDir, dynamicConfig);
+        assertThat(configuration.get(ConfigOptions.COORDINATOR_HOST)).isEqualTo("example.com");
+
+        // old and new common configuration file should not be present at the same time
+        Path commonYaml = tempFolder.resolve("common.yaml");
+        Files.write(commonYaml, Collections.singleton("bind.listeners: FLUSS://localhost:9124"));
+        assertThatThrownBy(() -> GlobalConfiguration.loadConfiguration(confDir, null))
+                .isInstanceOf(IllegalConfigurationException.class)
+                .hasMessageContaining("Only one of");
+
+        // new common configuration file
+        Files.delete(serverYaml);
+        configuration = GlobalConfiguration.loadConfiguration(confDir, null);
+        assertThat(configuration.get(ConfigOptions.BIND_LISTENERS))
+                .isEqualTo("FLUSS://localhost:9124");
+        assertThat(configuration.get(ConfigOptions.COORDINATOR_HOST)).isEqualTo(null);
+
+        // new common configuration file + precedence for dynamic properties
+        dynamicConfig = new Configuration();
+        dynamicConfig.set(ConfigOptions.BIND_LISTENERS, "FLUSS://example.com:9124");
+        configuration = GlobalConfiguration.loadConfiguration(confDir, dynamicConfig);
+        assertThat(configuration.get(ConfigOptions.BIND_LISTENERS))
+                .isEqualTo("FLUSS://example.com:9124");
+        assertThat(configuration.get(ConfigOptions.COORDINATOR_HOST)).isEqualTo(null);
+    }
+
+    @Test
+    void testLoadConfigurationWithAdditionalFiles(@TempDir Path tempFolder) throws Exception {
+        String confDir = tempFolder.toAbsolutePath().toString();
+        Path serverYaml = tempFolder.resolve("server.yaml");
+        Files.write(serverYaml, Collections.singleton("coordinator.host: localhost"));
+    }
+}
diff --git a/fluss-server/src/test/java/com/alibaba/fluss/server/utils/ConfigurationParserUtilsTest.java b/fluss-server/src/test/java/com/alibaba/fluss/server/utils/ConfigurationParserUtilsTest.java
@@ -20,30 +20,48 @@
 import com.alibaba.fluss.config.ConfigBuilder;
 import com.alibaba.fluss.config.ConfigOptions;
 import com.alibaba.fluss.config.Configuration;
-import com.alibaba.fluss.config.GlobalConfiguration;
+import com.alibaba.fluss.exception.IllegalConfigurationException;
 import com.alibaba.fluss.server.exception.FlussParseException;
 
 import org.apache.commons.cli.MissingOptionException;
 import org.junit.jupiter.api.io.TempDir;
 import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.EnumSource;
+import org.junit.jupiter.params.provider.MethodSource;
 
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.Collections;
+import java.util.stream.Stream;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.assertj.core.api.Assertions.fail;
 
 /** Test for {@link ConfigurationParserUtils}. */
 public class ConfigurationParserUtilsTest {
 
+    private static final String OLD_COMMON_CONFIG_FILE_NAME = "server.yaml";
+    private static final String NEW_COMMON_CONFIG_FILE_NAME = "common.yaml";
+    private static final String COORDINATOR_SERVER_CONF_FILE_NAME = "coordinator-server.yaml";
+    private static final String TABLET_SERVER_CONF_FILE_NAME = "tablet-server.yaml";
+
     @ParameterizedTest
-    @EnumSource(ServerType.class)
-    void testLoadConfiguration(ServerType serverType, @TempDir Path tempFolder) throws Exception {
-        Path yamlFile = tempFolder.resolve("server.yaml");
-        Files.write(yamlFile, Collections.singleton("coordinator.port: 9124"));
+    @MethodSource("parameterizedTestConfiguration")
+    void testLoadWithCommonConfigurationTile(
+            ServerType serverType,
+            String commonConfigFileName,
+            String additionalConfigFileName,
+            @TempDir Path tempFolder)
+            throws Exception {
+        Path yamlFile = tempFolder.resolve(commonConfigFileName);
+        Files.write(yamlFile, Collections.singleton("bind.listeners: FLUSS://localhost:9124"));
+        if (commonConfigFileName.equals(NEW_COMMON_CONFIG_FILE_NAME)) {
+            Files.write(tempFolder.resolve(additionalConfigFileName), Collections.emptyList());
+        }
         String confDir = tempFolder.toAbsolutePath().toString();
+
         final String key = "key";
         final String value = "value";
         final String arg1 = "arg1";
@@ -61,21 +79,29 @@ void testLoadConfiguration(ServerType serverType, @TempDir Path tempFolder) thro
                 .isEqualTo(value);
 
         // should respect the configurations in the server.yaml
-        assertThat(configuration.getString(ConfigOptions.COORDINATOR_PORT)).isEqualTo("9124");
+        assertThat(configuration.getString(ConfigOptions.BIND_LISTENERS))
+                .isEqualTo("FLUSS://localhost:9124");
     }
 
     @ParameterizedTest
-    @EnumSource(ServerType.class)
-    void testLoadWithUserSpecifiedConfigFile(ServerType serverType, @TempDir Path tempFolder)
+    @MethodSource("parameterizedTestConfiguration")
+    void testLoadWithUserSpecifiedConfigFile(
+            ServerType serverType,
+            String commonConfigFileName,
+            String additionalConfigFileName,
+            @TempDir Path tempFolder)
             throws Exception {
-        Path yamlFile = tempFolder.resolve("server.yaml");
-        Files.write(yamlFile, Collections.singleton("coordinator.port: 9124"));
+        Path yamlFile = tempFolder.resolve(commonConfigFileName);
+        Files.write(yamlFile, Collections.singleton("bind.listeners: FLUSS://localhost:9124"));
+        if (commonConfigFileName.equals(NEW_COMMON_CONFIG_FILE_NAME)) {
+            Files.write(tempFolder.resolve(additionalConfigFileName), Collections.emptyList());
+        }
         String confDir = tempFolder.toAbsolutePath().toString();
 
         Path userDefinedConfigFile = tempFolder.resolve("user-defined-server.yaml");
-        Files.write(yamlFile, Collections.singleton("coordinator.port: 1000"));
+        Files.write(yamlFile, Collections.singleton("bind.listeners: FLUSS://example.com:1000"));
 
-        final String configKey = GlobalConfiguration.FLUSS_CONF_FILENAME[0];
+        final String configKey = commonConfigFileName;
         final String configValue = userDefinedConfigFile.toString();
 
         final String[] args = {
@@ -87,13 +113,13 @@ void testLoadWithUserSpecifiedConfigFile(ServerType serverType, @TempDir Path te
         // should use the configurations in the user-defined-server.yaml
         assertThat(
                         configuration.get(
-                                ConfigBuilder.key("coordinator.port").intType().noDefaultValue()))
-                .isEqualTo(1000);
+                                ConfigBuilder.key("bind.listeners").stringType().noDefaultValue()))
+                .isEqualTo("FLUSS://example.com:1000");
     }
 
     @ParameterizedTest
     @EnumSource(ServerType.class)
-    void testLoadConfigurationThrowException(ServerType serverType) {
+    void testMissingConfigDirOptionThrowsException(ServerType serverType) {
         // should throw exception when miss options 'c'('configDir')
         assertThatThrownBy(
                         () ->
@@ -107,4 +133,132 @@ void testLoadConfigurationThrowException(ServerType serverType) {
                 .isInstanceOf(MissingOptionException.class)
                 .hasMessageContaining("Missing required option: c");
     }
+
+    @ParameterizedTest
+    @MethodSource("parameterizedTestConfiguration")
+    void testOnlyLoadRelevantConfigFiles(
+            ServerType serverType,
+            String commonConfigFileName,
+            String additionalConfigFileName,
+            @TempDir Path tempFolder)
+            throws Exception {
+        Path commonYamlFile = tempFolder.resolve(commonConfigFileName);
+        Files.write(commonYamlFile, Collections.singleton("bind.listeners: FLUSS://common:9124"));
+
+        // make all possible config files available in config dir to check only the relevant ones
+        // are loaded
+        if (serverType.equals(ServerType.COORDINATOR)) {
+            Files.write(
+                    tempFolder.resolve(additionalConfigFileName),
+                    Collections.singleton("bind.listeners: FLUSS://dedicated:9124"));
+            Files.write(
+                    tempFolder.resolve(TABLET_SERVER_CONF_FILE_NAME),
+                    Collections.singleton(
+                            "bind.listeners: FLUSS://expected-to-read-only-coordinator-server-yaml:9124"));
+        } else {
+            Files.write(
+                    tempFolder.resolve(additionalConfigFileName),
+                    Collections.singleton("bind.listeners: FLUSS://dedicated:9124"));
+            Files.write(
+                    tempFolder.resolve(COORDINATOR_SERVER_CONF_FILE_NAME),
+                    Collections.singleton(
+                            "bind.listeners: FLUSS://expected-to-read-only-tablet-server-yaml:9124"));
+        }
+        // in addition, put another YAML file on the config dir
+        Files.write(
+                tempFolder.resolve("conf.yaml"),
+                Collections.singleton(
+                        "bind.listeners: FLUSS://expected-conf-yaml-to-be-not-read:9124"));
+
+        String confDir = tempFolder.toAbsolutePath().toString();
+
+        String[] args = {"--configDir", confDir};
+
+        Configuration configuration =
+                ConfigurationParserUtils.loadConfiguration(
+                        args, ConfigurationParserUtilsTest.class.getSimpleName(), serverType);
+
+        if (commonConfigFileName.equals(OLD_COMMON_CONFIG_FILE_NAME)) {
+            // backward compatability
+            // should only load from old file, no additional files should be read
+            assertThat(configuration.getString(ConfigOptions.BIND_LISTENERS))
+                    .isEqualTo("FLUSS://common:9124");
+        } else if (commonConfigFileName.equals(NEW_COMMON_CONFIG_FILE_NAME)) {
+            // when using new common config file, dedicated config files should overwrite options
+            // from common, but only the files for the respective server type should be read
+            assertThat(configuration.getString(ConfigOptions.BIND_LISTENERS))
+                    .isEqualTo("FLUSS://dedicated:9124");
+        } else {
+            fail("Unexpected common config file name: " + commonConfigFileName);
+        }
+
+        args =
+                new String[] {
+                    "--configDir",
+                    confDir,
+                    String.format(
+                            "-D%s=%s",
+                            "bind.listeners",
+                            "FLUSS://dynamic-option-should-overwrite-config-file:9124"),
+                    String.format("-D%s=%s", "key", 1234)
+                };
+
+        // dynamic configuration options should overwrite config file options
+        configuration =
+                ConfigurationParserUtils.loadConfiguration(
+                        args, ConfigurationParserUtilsTest.class.getSimpleName(), serverType);
+        assertThat(configuration.getString(ConfigOptions.BIND_LISTENERS))
+                .isEqualTo("FLUSS://dynamic-option-should-overwrite-config-file:9124");
+        assertThat(configuration.getInt(ConfigBuilder.key("key").intType().noDefaultValue()))
+                .isEqualTo(1234);
+    }
+
+    @ParameterizedTest
+    @EnumSource(ServerType.class)
+    void testUsingOldAndNewCommonConfigThrowsException(
+            ServerType serverType, @TempDir Path tempFolder) throws Exception {
+        Path oldYaml = tempFolder.resolve(OLD_COMMON_CONFIG_FILE_NAME);
+        Files.write(oldYaml, Collections.singleton("bind.listeners: FLUSS://localhost:9124"));
+        Path newYaml = tempFolder.resolve(NEW_COMMON_CONFIG_FILE_NAME);
+        Files.write(newYaml, Collections.singleton("bind.listeners: FLUSS://localhost:9124"));
+        String confDir = tempFolder.toAbsolutePath().toString();
+
+        final String key = "key";
+        final String value = "value";
+        final String arg1 = "arg1";
+        final String arg2 = "arg2";
+
+        final String[] args = {
+            "--configDir", confDir, String.format("-D%s=%s", key, value), arg1, arg2
+        };
+
+        assertThatThrownBy(
+                        () ->
+                                ConfigurationParserUtils.loadConfiguration(
+                                        args,
+                                        ConfigurationParserUtilsTest.class.getSimpleName(),
+                                        serverType))
+                .isInstanceOf(IllegalConfigurationException.class)
+                .hasMessageContaining("Only one of");
+    }
+
+    private static Stream<Arguments> parameterizedTestConfiguration() {
+        return Stream.of(
+                Arguments.of(
+                        ServerType.COORDINATOR,
+                        OLD_COMMON_CONFIG_FILE_NAME,
+                        COORDINATOR_SERVER_CONF_FILE_NAME),
+                Arguments.of(
+                        ServerType.TABLET_SERVER,
+                        OLD_COMMON_CONFIG_FILE_NAME,
+                        TABLET_SERVER_CONF_FILE_NAME),
+                Arguments.of(
+                        ServerType.COORDINATOR,
+                        NEW_COMMON_CONFIG_FILE_NAME,
+                        COORDINATOR_SERVER_CONF_FILE_NAME),
+                Arguments.of(
+                        ServerType.TABLET_SERVER,
+                        NEW_COMMON_CONFIG_FILE_NAME,
+                        TABLET_SERVER_CONF_FILE_NAME));
+    }
 }
