[CELEBORN-1550][FOLLOWUP] Support celeborn.dynamicConfig.store.backen…

…d with short name and backend implementation ### What changes were proposed in this pull request? Introduce `ConfigStore` to support `celeborn.dynamicConfig.store.backend` with short name and backend implementation. ### Why are the changes needed? `celeborn.dynamicConfig.store.backend` is allowed to be specified in two ways: - Using short names: Default available options are FS, DB. - Using the fully qualified class name of the backend implementation. Therefore, it's recommended to introduce `ConfigStore` based on SPI mechanism for `celeborn.dynamicConfig.store.backend` instead of `dynamicConfigStoreBackendShortNames` which could not add other short name of backend implementation for users. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? CI. Closes apache#2698 from SteNicholas/CELEBORN-1550. Authored-by: SteNicholas <programgeek@163.com> Signed-off-by: Shuang <lvshuang.xjs@alibaba-inc.com>
Jay-ju · Aug 23, 2024 · 9fefa66 · 9fefa66
1 parent 1da2ede
commit 9fefa66
Show file tree

Hide file tree

Showing 11 changed files with 138 additions and 43 deletions.
diff --git a/common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala b/common/src/main/scala/org/apache/celeborn/common/CelebornConf.scala
@@ -5262,10 +5262,12 @@ object CelebornConf extends Logging {
   val DYNAMIC_CONFIG_STORE_BACKEND: OptionalConfigEntry[String] =
     buildConf("celeborn.dynamicConfig.store.backend")
       .categories("master", "worker")
-      .doc("Store backend for dynamic config service. The backend can be specified in two ways:" +
-        " - Using short names: Default available options are FS, DB." +
-        " - Using the fully qualified class name of the backend implementation." +
-        "If not provided, it means that dynamic configuration is disabled.")
+      .doc(
+        "Store backend for dynamic config service. The store backend can be specified in two ways:" +
+          " - Using the short name of the store backend defined in the implementation of `ConfigStore#getName` " +
+          "whose return value can be mapped to the corresponding backend implementation. Available options: FS, DB." +
+          " - Using the service class name of the store backend implementation." +
+          "If not provided, it means that dynamic configuration is disabled.")
       .version("0.4.0")
       .stringConf
       .createOptional

diff --git a/docs/configuration/master.md b/docs/configuration/master.md
@@ -21,7 +21,7 @@ license: |
 | --- | ------- | --------- | ----------- | ----- | ---------- |
 | celeborn.cluster.name | default | false | Celeborn cluster name. | 0.5.0 |  | 
 | celeborn.dynamicConfig.refresh.interval | 120s | false | Interval for refreshing the corresponding dynamic config periodically. | 0.4.0 |  | 
-| celeborn.dynamicConfig.store.backend | &lt;undefined&gt; | false | Store backend for dynamic config service. The backend can be specified in two ways: - Using short names: Default available options are FS, DB. - Using the fully qualified class name of the backend implementation.If not provided, it means that dynamic configuration is disabled. | 0.4.0 |  | 
+| celeborn.dynamicConfig.store.backend | &lt;undefined&gt; | false | Store backend for dynamic config service. The store backend can be specified in two ways: - Using the short name of the store backend defined in the implementation of `ConfigStore#getName` whose return value can be mapped to the corresponding backend implementation. Available options: FS, DB. - Using the service class name of the store backend implementation.If not provided, it means that dynamic configuration is disabled. | 0.4.0 |  | 
 | celeborn.dynamicConfig.store.db.fetch.pageSize | 1000 | false | The page size for db store to query configurations. | 0.5.0 |  | 
 | celeborn.dynamicConfig.store.db.hikari.connectionTimeout | 30s | false | The connection timeout that a client will wait for a connection from the pool for db store backend. | 0.5.0 |  | 
 | celeborn.dynamicConfig.store.db.hikari.driverClassName |  | false | The jdbc driver class name of db store backend. | 0.5.0 |  | 

diff --git a/docs/configuration/worker.md b/docs/configuration/worker.md
@@ -21,7 +21,7 @@ license: |
 | --- | ------- | --------- | ----------- | ----- | ---------- |
 | celeborn.cluster.name | default | false | Celeborn cluster name. | 0.5.0 |  | 
 | celeborn.dynamicConfig.refresh.interval | 120s | false | Interval for refreshing the corresponding dynamic config periodically. | 0.4.0 |  | 
-| celeborn.dynamicConfig.store.backend | &lt;undefined&gt; | false | Store backend for dynamic config service. The backend can be specified in two ways: - Using short names: Default available options are FS, DB. - Using the fully qualified class name of the backend implementation.If not provided, it means that dynamic configuration is disabled. | 0.4.0 |  | 
+| celeborn.dynamicConfig.store.backend | &lt;undefined&gt; | false | Store backend for dynamic config service. The store backend can be specified in two ways: - Using the short name of the store backend defined in the implementation of `ConfigStore#getName` whose return value can be mapped to the corresponding backend implementation. Available options: FS, DB. - Using the service class name of the store backend implementation.If not provided, it means that dynamic configuration is disabled. | 0.4.0 |  | 
 | celeborn.dynamicConfig.store.db.fetch.pageSize | 1000 | false | The page size for db store to query configurations. | 0.5.0 |  | 
 | celeborn.dynamicConfig.store.db.hikari.connectionTimeout | 30s | false | The connection timeout that a client will wait for a connection from the pool for db store backend. | 0.5.0 |  | 
 | celeborn.dynamicConfig.store.db.hikari.driverClassName |  | false | The jdbc driver class name of db store backend. | 0.5.0 |  | 

diff --git a/service/src/main/java/org/apache/celeborn/server/common/service/config/ConfigService.java b/service/src/main/java/org/apache/celeborn/server/common/service/config/ConfigService.java
@@ -110,11 +110,4 @@ default DynamicConfig getTenantUserConfigFromCache(String tenantId, String userI
 
   /** Shutdowns configuration management service. */
   void shutdown();
-
-  /**
-   * Retrieves the name of the configuration service.
-   *
-   * @return The name of the configuration service
-   */
-  String getName();
 }
diff --git a/service/src/main/java/org/apache/celeborn/server/common/service/config/ConfigStore.java b/service/src/main/java/org/apache/celeborn/server/common/service/config/ConfigStore.java
@@ -0,0 +1,39 @@
+/*
+ * 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.celeborn.server.common.service.config;
+
+/**
+ * Config store defines store backend for dynamic config service including short name and config
+ * service implementation.
+ */
+public interface ConfigStore {
+
+  /**
+   * Gets short name of store backend for dynamic config service.
+   *
+   * @return The short name of store backend for dynamic config service.
+   */
+  String getName();
+
+  /**
+   * Gets service class name of store backend for dynamic config service.
+   *
+   * @return The service class name of store backend for dynamic config service.
+   */
+  String getService();
+}
diff --git a/...e/src/main/java/org/apache/celeborn/server/common/service/config/DbConfigServiceImpl.java b/...e/src/main/java/org/apache/celeborn/server/common/service/config/DbConfigServiceImpl.java
@@ -57,11 +57,6 @@ public void refreshCache() throws IOException {
                     Function.identity())));
   }
 
-  @Override
-  public String getName() {
-    return "DB";
-  }
-
   @VisibleForTesting
   public IServiceManager getServiceManager() {
     return iServiceManager;

diff --git a/...ice/src/main/java/org/apache/celeborn/server/common/service/config/DbConfigStoreImpl.java b/...ice/src/main/java/org/apache/celeborn/server/common/service/config/DbConfigStoreImpl.java
@@ -0,0 +1,31 @@
+/*
+ * 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.celeborn.server.common.service.config;
+
+public class DbConfigStoreImpl implements ConfigStore {
+
+  @Override
+  public String getName() {
+    return "DB";
+  }
+
+  @Override
+  public String getService() {
+    return DbConfigServiceImpl.class.getName();
+  }
+}
diff --git a/...in/java/org/apache/celeborn/server/common/service/config/DynamicConfigServiceFactory.java b/...in/java/org/apache/celeborn/server/common/service/config/DynamicConfigServiceFactory.java
@@ -17,40 +17,30 @@
 
 package org.apache.celeborn.server.common.service.config;
 
-import java.io.IOException;
-import java.util.HashMap;
-import java.util.Locale;
+import java.util.ServiceLoader;
 
 import org.apache.celeborn.common.CelebornConf;
 import org.apache.celeborn.common.util.Utils;
 
 public class DynamicConfigServiceFactory {
   private static volatile ConfigService _INSTANCE;
 
-  // short names for dynamic config store backends
-  private static final HashMap<String, String> dynamicConfigStoreBackendShortNames =
-      new HashMap<String, String>() {
-        {
-          put("FS", FsConfigServiceImpl.class.getName());
-          put("DB", DbConfigServiceImpl.class.getName());
-        }
-      };
-
-  public static ConfigService getConfigService(CelebornConf celebornConf) throws IOException {
+  public static ConfigService getConfigService(CelebornConf celebornConf) {
     if (celebornConf.dynamicConfigStoreBackend().isEmpty()) {
       return null;
     }
 
     if (_INSTANCE == null) {
       synchronized (DynamicConfigServiceFactory.class) {
         if (_INSTANCE == null) {
-          String configStoreBackendName = celebornConf.dynamicConfigStoreBackend().get();
-          String configStoreBackendClass =
-              dynamicConfigStoreBackendShortNames.getOrDefault(
-                  configStoreBackendName.toUpperCase(Locale.ROOT), configStoreBackendName);
-
-          _INSTANCE =
-              Utils.instantiateDynamicConfigStoreBackend(configStoreBackendClass, celebornConf);
+          String configStoreBackend = celebornConf.dynamicConfigStoreBackend().get();
+          for (ConfigStore configStore : ServiceLoader.load(ConfigStore.class)) {
+            if (configStore.getName().equalsIgnoreCase(configStoreBackend)) {
+              configStoreBackend = configStore.getService();
+              break;
+            }
+          }
+          _INSTANCE = Utils.instantiateDynamicConfigStoreBackend(configStoreBackend, celebornConf);
         }
       }
     }

diff --git a/...e/src/main/java/org/apache/celeborn/server/common/service/config/FsConfigServiceImpl.java b/...e/src/main/java/org/apache/celeborn/server/common/service/config/FsConfigServiceImpl.java
@@ -104,9 +104,4 @@ private File getConfigFile(Map<String, String> env) throws IOException {
     }
     return configFile;
   }
-
-  @Override
-  public String getName() {
-    return "FS";
-  }
 }
diff --git a/...ice/src/main/java/org/apache/celeborn/server/common/service/config/FsConfigStoreImpl.java b/...ice/src/main/java/org/apache/celeborn/server/common/service/config/FsConfigStoreImpl.java
@@ -0,0 +1,31 @@
+/*
+ * 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.celeborn.server.common.service.config;
+
+public class FsConfigStoreImpl implements ConfigStore {
+
+  @Override
+  public String getName() {
+    return "FS";
+  }
+
+  @Override
+  public String getService() {
+    return FsConfigServiceImpl.class.getName();
+  }
+}
diff --git a/.../resources/META-INF/services/org.apache.celeborn.server.common.service.config.ConfigStore b/.../resources/META-INF/services/org.apache.celeborn.server.common.service.config.ConfigStore
@@ -0,0 +1,19 @@
+#
+# 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.
+#
+
+org.apache.celeborn.server.common.service.config.FsConfigStoreImpl
+org.apache.celeborn.server.common.service.config.DbConfigStoreImpl