Adding support to register settings dynamically (#5495) (#5544)

* Adding support to register settings dynamically Signed-off-by: Ryan Bogan <[email protected]> * Update CHANGELOG Signed-off-by: Ryan Bogan <[email protected]> * Removed unnecessary registerSetting methods Signed-off-by: Ryan Bogan <[email protected]> * Change setting registration order Signed-off-by: Ryan Bogan <[email protected]> * Add unregisterSettings method Signed-off-by: Ryan Bogan <[email protected]> * Remove unnecessary feature flag Signed-off-by: Ryan Bogan <[email protected]> Signed-off-by: Ryan Bogan <[email protected]> (cherry picked from commit 08cd06f) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Signed-off-by: Ryan Bogan <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-project · Dec 13, 2022 · 0ea42f2 · 0ea42f2
1 parent 816e12c
commit 0ea42f2
Show file tree

Hide file tree

Showing 5 changed files with 104 additions and 2 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -14,6 +14,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Add max_shard_size parameter for shrink API ([#5229](https://github.com/opensearch-project/OpenSearch/pull/5229))
 - Added jackson dependency to server ([#5366] (https://github.com/opensearch-project/OpenSearch/pull/5366))
 - Added experimental extensions to main ([#5347](https://github.com/opensearch-project/OpenSearch/pull/5347))
+- Adding support to register settings dynamically ([#5495](https://github.com/opensearch-project/OpenSearch/pull/5495))
 
 ### Dependencies
 - Bump bcpg-fips from 1.0.5.1 to 1.0.7.1 ([#5148](https://github.com/opensearch-project/OpenSearch/pull/5148))

diff --git a/server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java b/server/src/main/java/org/opensearch/common/settings/AbstractScopedSettings.java
@@ -121,8 +121,8 @@ protected AbstractScopedSettings(
                 keySettings.putIfAbsent(setting.getKey(), setting);
             }
         }
-        this.complexMatchers = Collections.unmodifiableMap(complexMatchers);
-        this.keySettings = Collections.unmodifiableMap(keySettings);
+        this.complexMatchers = complexMatchers;
+        this.keySettings = keySettings;
     }
 
     protected void validateSettingKey(Setting<?> setting) {
@@ -144,6 +144,23 @@ protected AbstractScopedSettings(Settings nodeSettings, Settings scopeSettings,
         settingUpdaters.addAll(other.settingUpdaters);
     }
 
+    public boolean registerSetting(Setting<?> setting) {
+        validateSettingKey(setting);
+        if (setting.hasComplexMatcher()) {
+            return setting != complexMatchers.putIfAbsent(setting.getKey(), setting);
+        } else {
+            return setting != keySettings.putIfAbsent(setting.getKey(), setting);
+        }
+    }
+
+    public boolean unregisterSetting(Setting<?> setting) {
+        if (setting.hasComplexMatcher()) {
+            return setting != complexMatchers.remove(setting.getKey());
+        } else {
+            return setting != keySettings.remove(setting.getKey());
+        }
+    }
+
     /**
      * Returns <code>true</code> iff the given key is a valid settings key otherwise <code>false</code>
      */

diff --git a/server/src/main/java/org/opensearch/common/settings/SettingsModule.java b/server/src/main/java/org/opensearch/common/settings/SettingsModule.java
@@ -179,6 +179,45 @@ public void configure(Binder binder) {
         binder.bind(IndexScopedSettings.class).toInstance(indexScopedSettings);
     }
 
+    /**
+     * Dynamically registers a new Setting at Runtime. This method is mostly used by plugins/extensions
+     * to register new settings at runtime. Settings can be of Node Scope or Index Scope.
+     * @param setting which is being registered in the cluster.
+     * @return boolean value is set to true when successfully registered, else returns false
+     */
+    public boolean registerDynamicSetting(Setting<?> setting) {
+        boolean onNodeSetting = false;
+        boolean onIndexSetting = false;
+        try {
+            if (setting.hasNodeScope()) {
+                onNodeSetting = clusterSettings.registerSetting(setting);
+            }
+            if (setting.hasIndexScope()) {
+                onIndexSetting = indexScopedSettings.registerSetting(setting);
+            }
+            try {
+                registerSetting(setting);
+                if (onNodeSetting || onIndexSetting) {
+                    logger.info("Registered new Setting: " + setting.getKey() + " successfully ");
+                    return true;
+                }
+            } catch (IllegalArgumentException ex) {
+                if (onNodeSetting) {
+                    clusterSettings.unregisterSetting(setting);
+                }
+
+                if (onIndexSetting) {
+                    indexScopedSettings.unregisterSetting(setting);
+                }
+                throw ex;
+            }
+        } catch (Exception e) {
+            logger.error("Could not register setting " + setting.getKey());
+            throw new SettingsException("Could not register setting:" + setting.getKey());
+        }
+        return false;
+    }
+
     /**
      * Registers a new setting. This method should be used by plugins in order to expose any custom settings the plugin defines.
      * Unless a setting is registered the setting is unusable. If a setting is never the less specified the node will reject

diff --git a/server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java b/server/src/test/java/org/opensearch/common/settings/SettingsModuleTests.java
@@ -34,6 +34,7 @@
 
 import org.opensearch.common.inject.ModuleTestCase;
 import org.opensearch.common.settings.Setting.Property;
+import org.opensearch.common.util.FeatureFlagTests;
 import org.hamcrest.Matchers;
 
 import java.util.Arrays;
@@ -237,4 +238,47 @@ public void testOldMaxClauseCountSetting() {
             ex.getMessage()
         );
     }
+
+    public void testDynamicNodeSettingsRegistration() {
+        FeatureFlagTests.enableFeature();
+        Settings settings = Settings.builder().put("some.custom.setting", "2.0").build();
+        SettingsModule module = new SettingsModule(settings, Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope));
+        assertNotNull(module.getClusterSettings().get("some.custom.setting"));
+        // For unregistered setting the value is expected to be null
+        assertNull(module.getClusterSettings().get("some.custom.setting2"));
+        assertInstanceBinding(module, Settings.class, (s) -> s == settings);
+
+        assertTrue(module.registerDynamicSetting(Setting.floatSetting("some.custom.setting2", 1.0f, Property.NodeScope)));
+        assertNotNull(module.getClusterSettings().get("some.custom.setting2"));
+        // verify if some.custom.setting still exists
+        assertNotNull(module.getClusterSettings().get("some.custom.setting"));
+
+        // verify exception is thrown when setting registration fails
+        expectThrows(
+            SettingsException.class,
+            () -> module.registerDynamicSetting(Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope))
+        );
+    }
+
+    public void testDynamicIndexSettingsRegistration() {
+        FeatureFlagTests.enableFeature();
+        Settings settings = Settings.builder().put("some.custom.setting", "2.0").build();
+        SettingsModule module = new SettingsModule(settings, Setting.floatSetting("some.custom.setting", 1.0f, Property.NodeScope));
+        assertNotNull(module.getClusterSettings().get("some.custom.setting"));
+        // For unregistered setting the value is expected to be null
+        assertNull(module.getIndexScopedSettings().get("index.custom.setting2"));
+        assertInstanceBinding(module, Settings.class, (s) -> s == settings);
+
+        assertTrue(module.registerDynamicSetting(Setting.floatSetting("index.custom.setting2", 1.0f, Property.IndexScope)));
+        assertNotNull(module.getIndexScopedSettings().get("index.custom.setting2"));
+
+        // verify if some.custom.setting still exists
+        assertNotNull(module.getClusterSettings().get("some.custom.setting"));
+
+        // verify exception is thrown when setting registration fails
+        expectThrows(
+            SettingsException.class,
+            () -> module.registerDynamicSetting(Setting.floatSetting("index.custom.setting2", 1.0f, Property.IndexScope))
+        );
+    }
 }
diff --git a/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java b/server/src/test/java/org/opensearch/common/util/FeatureFlagTests.java
@@ -48,4 +48,5 @@ public void testRemoteStoreFeatureFlag() {
         assertNotNull(System.getProperty(remoteStoreFlag));
         assertTrue(FeatureFlags.isEnabled(remoteStoreFlag));
     }
+
 }
-Original file line number
+Diff line change
@@ Expand Up / @@ -48,4 +48,5 @@ public void testRemoteStoreFeatureFlag() { @@
             assertNotNull(System.getProperty(remoteStoreFlag));
             assertTrue(FeatureFlags.isEnabled(remoteStoreFlag));
         }
     }