[CCI] [BUG] Fixing extension settings update consumers (opensearch-pr…

…oject#7456) * add equels for ClusterSettings Signed-off-by: Kuanysh <[email protected]> * added junit Signed-off-by: Kuanysh <[email protected]> * code refactoring Signed-off-by: Kuanysh <[email protected]> * added changes to handleAddSettingsUpdateConsumer Signed-off-by: Kuanysh <[email protected]> * code refactoring Signed-off-by: Kuanysh Aimurzinov <[email protected]> * code refactoring Signed-off-by: Kuanysh Aimurzinov <[email protected]> * changed main method Signed-off-by: Kuanysh <[email protected]> --------- Signed-off-by: Kuanysh <[email protected]> Signed-off-by: Kuanysh <[email protected]> Signed-off-by: Kuanysh Aimurzinov <[email protected]> Signed-off-by: Shivansh Arora <[email protected]>
Gaurav614 · Apr 25, 2024 · c5fee5a · c5fee5a
1 parent 5015cc8
commit c5fee5a
Show file tree

Hide file tree

Showing 3 changed files with 64 additions and 15 deletions.
diff --git a/server/src/main/java/org/opensearch/extensions/AddSettingsUpdateConsumerRequestHandler.java b/server/src/main/java/org/opensearch/extensions/AddSettingsUpdateConsumerRequestHandler.java
@@ -15,6 +15,7 @@
 import org.opensearch.cluster.service.ClusterService;
 import org.opensearch.common.settings.Setting;
 import org.opensearch.common.settings.SettingsException;
+import org.opensearch.common.settings.SettingsModule;
 import org.opensearch.common.settings.WriteableSetting;
 import org.opensearch.transport.TransportResponse;
 import org.opensearch.transport.TransportService;
@@ -31,6 +32,7 @@ public class AddSettingsUpdateConsumerRequestHandler {
     private final ClusterService clusterService;
     private final TransportService transportService;
     private final String updateSettingsRequestType;
+    private final SettingsModule settingsModule;
 
     /**
      * Instantiates a new Add Settings Update Consumer Request Handler with the {@link ClusterService} and {@link TransportService}
@@ -42,11 +44,13 @@ public class AddSettingsUpdateConsumerRequestHandler {
     public AddSettingsUpdateConsumerRequestHandler(
         ClusterService clusterService,
         TransportService transportService,
-        String updateSettingsRequestType
+        String updateSettingsRequestType,
+        SettingsModule settingsModule
     ) {
         this.clusterService = clusterService;
         this.transportService = transportService;
         this.updateSettingsRequestType = updateSettingsRequestType;
+        this.settingsModule = settingsModule;
     }
 
     /**
@@ -68,25 +72,53 @@ TransportResponse handleAddSettingsUpdateConsumerRequest(AddSettingsUpdateConsum
 
                 // Extract setting and type from writeable setting
                 Setting<?> setting = extensionComponentSetting.getSetting();
+
+                // we need to get the actual setting from nodeSetting or indexsetting maps in SettingsModule
+                // use conditional based on setting properties
+                Setting<?> settingForUpdateConsumer = null;
+                if (setting.hasIndexScope()) {
+                    settingForUpdateConsumer = settingsModule.getIndexScopedSettings().get(setting.getKey());
+                } else if (setting.hasNodeScope()) {
+                    settingForUpdateConsumer = settingsModule.getClusterSettings().get(setting.getKey());
+                }
+                // do a null check and throw IllegalArgument exception here if neither index or node scope
+                if (settingForUpdateConsumer == null) {
+                    throw new IllegalArgumentException("Not index or node scope");
+                }
+
                 WriteableSetting.SettingType settingType = extensionComponentSetting.getType();
 
-                // Register setting update consumer with callback method to extension
-                clusterService.getClusterSettings().addSettingsUpdateConsumer(setting, (data) -> {
-                    logger.debug("Sending extension request type: " + updateSettingsRequestType);
-                    UpdateSettingsResponseHandler updateSettingsResponseHandler = new UpdateSettingsResponseHandler();
-                    transportService.sendRequest(
-                        extensionNode,
-                        updateSettingsRequestType,
-                        new UpdateSettingsRequest(settingType, setting, data),
-                        updateSettingsResponseHandler
-                    );
-                });
+                if (setting.hasIndexScope()) {
+                    // Register setting update consumer with callback method to extension
+                    clusterService.getClusterSettings().addSettingsUpdateConsumer(settingForUpdateConsumer, (data) -> {
+                        logger.debug("Sending extension request type: " + updateSettingsRequestType);
+                        UpdateSettingsResponseHandler updateSettingsResponseHandler = new UpdateSettingsResponseHandler();
+                        transportService.sendRequest(
+                            extensionNode,
+                            updateSettingsRequestType,
+                            new UpdateSettingsRequest(settingType, setting, data),
+                            updateSettingsResponseHandler
+                        );
+                    });
+                }
+                if (setting.hasNodeScope()) {
+                    // Register setting update consumer with callback method to extension
+                    clusterService.getClusterSettings().addSettingsUpdateConsumer(settingForUpdateConsumer, (data) -> {
+                        logger.debug("Sending extension request type: " + updateSettingsRequestType);
+                        UpdateSettingsResponseHandler updateSettingsResponseHandler = new UpdateSettingsResponseHandler();
+                        transportService.sendRequest(
+                            extensionNode,
+                            updateSettingsRequestType,
+                            new UpdateSettingsRequest(settingType, setting, data),
+                            updateSettingsResponseHandler
+                        );
+                    });
+                }
             }
-        } catch (SettingsException e) {
+        } catch (SettingsException | IllegalArgumentException e) {
             logger.error(e.toString());
             status = false;
         }
-
         return new AcknowledgedResponse(status);
     }
 }
diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java
@@ -172,7 +172,8 @@ public void initializeServicesAndRestHandler(
         this.addSettingsUpdateConsumerRequestHandler = new AddSettingsUpdateConsumerRequestHandler(
             clusterService,
             transportService,
-            REQUEST_EXTENSION_UPDATE_SETTINGS
+            REQUEST_EXTENSION_UPDATE_SETTINGS,
+            settingsModule
         );
         this.client = client;
         this.extensionTransportActionsHandler = new ExtensionTransportActionsHandler(

diff --git a/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java b/server/src/test/java/org/opensearch/common/settings/ScopedSettingsTests.java
@@ -1462,4 +1462,20 @@ public List<String> getListValue(final List<String> value) {
         );
     }
 
+    public void testAddSettingsUpdateConsumer() {
+        Setting<Integer> testSetting = Setting.intSetting("foo.bar", 1, Property.Dynamic, Property.NodeScope);
+        Setting<Integer> testSetting2 = Setting.intSetting("foo.bar.baz", 1, Property.Dynamic, Property.NodeScope);
+        AbstractScopedSettings service = new ClusterSettings(Settings.EMPTY, new HashSet<>(Arrays.asList(testSetting, testSetting2)));
+        AtomicInteger consumer2 = new AtomicInteger();
+        service.addSettingsUpdateConsumer(testSetting2, consumer2::set, (s) -> assertTrue(s > 0));
+        Setting<Integer> wrongKeySetting = Setting.intSetting("foo.bar.wrong", 1, Property.Dynamic, Property.NodeScope);
+
+        expectThrows(SettingsException.class, () -> service.addSettingsUpdateConsumer(wrongKeySetting, consumer2::set, (i) -> {
+            if (i == 42) throw new AssertionError("wrong key");
+        }));
+
+        expectThrows(NullPointerException.class, () -> service.addSettingsUpdateConsumer(null, consumer2::set, (i) -> {
+            if (i == 42) throw new AssertionError("empty key");
+        }));
+    }
 }