baiyangtx commented on code in PR #4072:
URL: https://github.com/apache/amoro/pull/4072#discussion_r2767903383
##########
amoro-common/src/main/java/org/apache/amoro/ActivePlugin.java:
##########
@@ -30,6 +32,19 @@ public interface ActivePlugin extends AmoroPlugin {
*/
void open(Map<String, String> properties);
+ /**
+ * Initialize and open the plugin with a {@link ConfigurationManager}.
+ *
+ * <p>The default implementation delegates to {@link #open(Map)} to keep
existing plugins
+ * compatible. New plugins can override this method to make use of dynamic
configuration.
+ *
+ * @param properties plugin properties
+ * @param configurationManager configuration manager used to fetch dynamic
overrides
+ */
+ default void open(Map<String, String> properties, ConfigurationManager
configurationManager) {
Review Comment:
I suggest using `void open(Configuration configs)` instead.
##########
amoro-ams/src/main/java/org/apache/amoro/server/terminal/TerminalManager.java:
##########
@@ -93,6 +93,12 @@ public TerminalManager(Configurations conf, CatalogManager
catalogManager) {
gcThread.start();
}
+ public TerminalManager(
+ org.apache.amoro.config.DynamicConfigurations dynamicConfigurations,
+ CatalogManager catalogManager) {
+ this((Configurations) dynamicConfigurations, catalogManager);
+ }
+
Review Comment:
1. No need type casting.
2. Import at file head.
##########
amoro-ams/src/main/java/org/apache/amoro/server/manager/AbstractPluginManager.java:
##########
@@ -69,11 +72,28 @@ public abstract class AbstractPluginManager<T extends
ActivePlugin> implements P
private final Map<String, T> foundedPlugins = new ConcurrentHashMap<>();
private final Map<String, PluginConfiguration> pluginConfigs =
Maps.newLinkedHashMap();
private final String pluginCategory;
+ protected final Configurations serviceConfig;
private final Class<T> pluginType;
+ private final ConfigurationManager configurationManager;
@SuppressWarnings("unchecked")
public AbstractPluginManager(String pluginCategory) {
+ this(pluginCategory, null, null);
+ }
Review Comment:
I think this constructor may should be removed.
##########
amoro-common/src/main/java/org/apache/amoro/config/DynamicConfigurations.java:
##########
@@ -0,0 +1,198 @@
+/*
+ * 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.amoro.config;
+
+import java.time.Duration;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Optional;
+
+/**
+ * DynamicConfigurations represents a configuration view that can be updated
at runtime.
+ *
+ * <p>Each instance keeps a reference to the initial static {@link
Configurations} (baseConfig) and
+ * asks a {@link ConfigurationManager} for service level or plugin level
overrides during refresh.
+ * The effective configuration is always
+ *
+ * <pre>
+ * currentConfig = merge(baseConfig, overridesFromManager)
+ * </pre>
+ *
+ * where overrides come from:
+ *
+ * <ul>
+ * <li>AMS service level: {@link
ConfigurationManager#getServerConfigurations()}.
+ * <li>Plugin level: {@link
ConfigurationManager#getPluginConfigurations(String, String)}.
+ * </ul>
+ *
+ * <p>This class extends {@link Configurations} so that it can be injected
wherever a {@code
+ * Configurations} is expected. All getter methods defined in {@link
Configurations} operate on the
+ * current merged configuration.
+ */
+public class DynamicConfigurations extends Configurations {
Review Comment:
DynamicConfigurations and ConfigurationManager should be in ams module.
##########
amoro-ams/src/main/java/org/apache/amoro/server/process/ProcessService.java:
##########
@@ -76,6 +76,16 @@ public ProcessService(Configurations serviceConfig,
TableService tableService) {
this(serviceConfig, tableService, new ActionCoordinatorManager(), new
ExecuteEngineManager());
}
+ public ProcessService(
+ org.apache.amoro.config.DynamicConfigurations dynamicConfigurations,
Review Comment:
Import class at file head.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
