Addressed PR Comments

Signed-off-by: Ryan Bogan <rbogan@amazon.com>
opensearch-project · Dec 20, 2022 · b349762 · b349762
1 parent aab1306
commit b349762
Show file tree

Hide file tree

Showing 7 changed files with 15 additions and 35 deletions.
diff --git a/server/src/main/java/org/opensearch/common/settings/WriteableSetting.java b/server/src/main/java/org/opensearch/common/settings/WriteableSetting.java
@@ -101,16 +101,12 @@ public WriteableSetting(StreamInput in) throws IOException {
      * @return The corresponding {@link SettingType} for the default value.
      */
     private static SettingType getGenericTypeFromDefault(Setting<?> setting) {
-        String typeStr = null;
+        String typeStr = setting.getDefault(Settings.EMPTY).getClass().getSimpleName();
         try {
-            // This throws NPE on null default
-            typeStr = setting.getDefault(Settings.EMPTY).getClass().getSimpleName();
             // This throws IAE if not in enum
             return SettingType.valueOf(typeStr);
-        } catch (NullPointerException e) {
-            throw new IllegalArgumentException("Unable to determine the generic type of this setting with a null default value.");
         } catch (IllegalArgumentException e) {
-            throw new UnsupportedOperationException(
+            throw new IllegalArgumentException(
                 "This class is not yet set up to handle the generic type: "
                     + typeStr
                     + ". Supported types are "

diff --git a/server/src/main/java/org/opensearch/extensions/AddSettingsUpdateConsumerRequest.java b/server/src/main/java/org/opensearch/extensions/AddSettingsUpdateConsumerRequest.java
@@ -44,11 +44,10 @@ public AddSettingsUpdateConsumerRequest(StreamInput in) throws IOException {
 
         // Read in component setting list
         int componentSettingsCount = in.readVInt();
-        List<WriteableSetting> componentSettings = new ArrayList<>(componentSettingsCount);
+        this.componentSettings = new ArrayList<>(componentSettingsCount);
         for (int i = 0; i < componentSettingsCount; i++) {
-            componentSettings.add(new WriteableSetting(in));
+            this.componentSettings.add(new WriteableSetting(in));
         }
-        this.componentSettings = componentSettings;
     }
 
     @Override
@@ -75,11 +74,7 @@ public DiscoveryExtensionNode getExtensionNode() {
 
     @Override
     public String toString() {
-        return "AddSettingsUpdateConsumerRequest{extensionNode="
-            + this.extensionNode.toString()
-            + ", componentSettings="
-            + this.componentSettings.toString()
-            + "}";
+        return "AddSettingsUpdateConsumerRequest{extensionNode=" + this.extensionNode.toString() + "}";
     }
 
     @Override

diff --git a/server/src/main/java/org/opensearch/extensions/AddSettingsUpdateConsumerRequestHandler.java b/server/src/main/java/org/opensearch/extensions/AddSettingsUpdateConsumerRequestHandler.java
@@ -71,7 +71,7 @@ TransportResponse handleAddSettingsUpdateConsumerRequest(AddSettingsUpdateConsum
 
                 // Register setting update consumer with callback method to extension
                 clusterService.getClusterSettings().addSettingsUpdateConsumer(setting, (data) -> {
-                    logger.info("Sending extension request type: " + updateSettingsRequestType);
+                    logger.debug("Sending extension request type: " + updateSettingsRequestType);
                     UpdateSettingsResponseHandler updateSettingsResponseHandler = new UpdateSettingsResponseHandler();
                     transportService.sendRequest(
                         extensionNode,

diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionActionListener.java b/server/src/main/java/org/opensearch/extensions/ExtensionActionListener.java
@@ -9,6 +9,8 @@
 package org.opensearch.extensions;
 
 import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
 
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
@@ -23,7 +25,7 @@
 public class ExtensionActionListener implements ActionListener<Response> {
 
     private static final Logger logger = LogManager.getLogger(ExtensionActionListener.class);
-    private ArrayList<Exception> exceptionList;
+    private List<Exception> exceptionList;
 
     public ExtensionActionListener() {
         exceptionList = new ArrayList<Exception>();
@@ -40,11 +42,7 @@ public void onFailure(Exception e) {
         logger.error(e.getMessage());
     }
 
-    public static Logger getLogger() {
-        return logger;
-    }
-
-    public ArrayList<Exception> getExceptionList() {
-        return exceptionList;
+    public List<Exception> getExceptionList() {
+        return Collections.unmodifiableList(exceptionList);
     }
 }
diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionActionListenerOnFailureRequest.java b/server/src/main/java/org/opensearch/extensions/ExtensionActionListenerOnFailureRequest.java
@@ -8,8 +8,6 @@
 
 package org.opensearch.extensions;
 
-import org.apache.logging.log4j.LogManager;
-import org.apache.logging.log4j.Logger;
 import org.opensearch.common.io.stream.StreamInput;
 import org.opensearch.common.io.stream.StreamOutput;
 import org.opensearch.transport.TransportRequest;
@@ -23,7 +21,6 @@
  * @opensearch.internal
  */
 public class ExtensionActionListenerOnFailureRequest extends TransportRequest {
-    private static final Logger logger = LogManager.getLogger(ExtensionRequest.class);
     private String failureExceptionMessage;
 
     /**

diff --git a/server/src/main/java/org/opensearch/extensions/settings/RegisterCustomSettingsRequest.java b/server/src/main/java/org/opensearch/extensions/settings/RegisterCustomSettingsRequest.java
@@ -37,12 +37,11 @@ public RegisterCustomSettingsRequest(StreamInput in) throws IOException {
         super(in);
         this.uniqueId = in.readString();
         int size = in.readVInt();
-        List<Setting<?>> settingsList = new ArrayList<>(size);
+        this.settings = new ArrayList<>(size);
         for (int i = 0; i < size; i++) {
             WriteableSetting ws = new WriteableSetting(in);
-            settingsList.add(ws.getSetting());
+            this.settings.add(ws.getSetting());
         }
-        this.settings = settingsList;
     }
 
     @Override

diff --git a/server/src/test/java/org/opensearch/common/settings/WriteableSettingTests.java b/server/src/test/java/org/opensearch/common/settings/WriteableSettingTests.java
@@ -471,17 +471,12 @@ public void testExceptionHandling() throws NoSuchFieldException, SecurityExcepti
         Field p = setting.getClass().getDeclaredField("parser");
         p.setAccessible(true);
 
-        // test null default value
-        dv.set(setting, null);
-        IllegalArgumentException iae = expectThrows(IllegalArgumentException.class, () -> new WriteableSetting(setting));
-        assertTrue(iae.getMessage().contains("null default value"));
-
         // test default value type not in enum
         Function<Settings, String> dvfi = s -> "";
         dv.set(setting, dvfi);
         Function<String, WriteableSettingTests> pfi = s -> new WriteableSettingTests();
         p.set(setting, pfi);
-        UnsupportedOperationException uoe = expectThrows(UnsupportedOperationException.class, () -> new WriteableSetting(setting));
-        assertTrue(uoe.getMessage().contains("generic type: WriteableSettingTests"));
+        IllegalArgumentException iae = expectThrows(IllegalArgumentException.class, () -> new WriteableSetting(setting));
+        assertTrue(iae.getMessage().contains("generic type: WriteableSettingTests"));
     }
 }