Address comments

Signed-off-by: Lakshya Taragi <[email protected]>
opensearch-project · Apr 8, 2024 · 6d434aa · 6d434aa
1 parent 8b05f9d
commit 6d434aa
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 30 deletions.
diff --git a/...rnalClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationAllocationIT.java b/...rnalClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationAllocationIT.java
@@ -387,15 +387,25 @@ public void initializeCluster(boolean remoteClusterManager) {
         client = internalCluster().client();
     }
 
+    // assign settings to be updated randomly as persistent or transient
+    private static void randomlyAssignPersistentOrTransient(Settings.Builder settingsBuilder) {
+        boolean isPersistentSetting = randomBoolean();
+        if (isPersistentSetting) {
+            updateSettingsRequest.persistentSettings(settingsBuilder);
+        } else {
+            updateSettingsRequest.transientSettings(settingsBuilder);
+        }
+    }
+
     // set the compatibility mode of cluster [strict, mixed]
     public static void setClusterMode(String mode) {
-        updateSettingsRequest.persistentSettings(Settings.builder().put(REMOTE_STORE_COMPATIBILITY_MODE_SETTING.getKey(), mode));
+        randomlyAssignPersistentOrTransient(Settings.builder().put(REMOTE_STORE_COMPATIBILITY_MODE_SETTING.getKey(), mode));
         assertAcked(internalCluster().client().admin().cluster().updateSettings(updateSettingsRequest).actionGet());
     }
 
     // set the migration direction for cluster [remote_store, docrep, none]
     public static void setDirection(String direction) {
-        updateSettingsRequest.persistentSettings(Settings.builder().put(MIGRATION_DIRECTION_SETTING.getKey(), direction));
+        randomlyAssignPersistentOrTransient(Settings.builder().put(MIGRATION_DIRECTION_SETTING.getKey(), direction));
         assertAcked(internalCluster().client().admin().cluster().updateSettings(updateSettingsRequest).actionGet());
     }
 

diff --git a/...va/org/opensearch/action/admin/cluster/settings/TransportClusterUpdateSettingsAction.java b/...va/org/opensearch/action/admin/cluster/settings/TransportClusterUpdateSettingsAction.java
@@ -36,7 +36,6 @@
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.message.ParameterizedMessage;
 import org.opensearch.OpenSearchException;
-import org.opensearch.Version;
 import org.opensearch.action.support.ActionFilters;
 import org.opensearch.action.support.clustermanager.TransportClusterManagerNodeAction;
 import org.opensearch.cluster.AckedClusterStateUpdateTask;
@@ -46,6 +45,7 @@
 import org.opensearch.cluster.metadata.IndexNameExpressionResolver;
 import org.opensearch.cluster.metadata.Metadata;
 import org.opensearch.cluster.node.DiscoveryNode;
+import org.opensearch.cluster.node.DiscoveryNodes;
 import org.opensearch.cluster.routing.allocation.AllocationService;
 import org.opensearch.cluster.service.ClusterManagerTaskKeys;
 import org.opensearch.cluster.service.ClusterManagerTaskThrottler;
@@ -54,6 +54,7 @@
 import org.opensearch.common.Priority;
 import org.opensearch.common.inject.Inject;
 import org.opensearch.common.settings.ClusterSettings;
+import org.opensearch.common.settings.Settings;
 import org.opensearch.common.settings.SettingsException;
 import org.opensearch.core.action.ActionListener;
 import org.opensearch.core.common.io.stream.StreamInput;
@@ -62,9 +63,7 @@
 import org.opensearch.transport.TransportService;
 
 import java.io.IOException;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Optional;
+import java.util.Locale;
 import java.util.Set;
 import java.util.stream.Collectors;
 
@@ -145,7 +144,6 @@ protected void clusterManagerOperation(
         final ClusterState state,
         final ActionListener<ClusterUpdateSettingsResponse> listener
     ) {
-        validateCompatibilityModeSettingRequest(request, state);
         final SettingsUpdater updater = new SettingsUpdater(clusterSettings);
         clusterService.submitStateUpdateTask(
             "cluster_update_settings",
@@ -260,6 +258,7 @@ public void onFailure(String source, Exception e) {
 
                 @Override
                 public ClusterState execute(final ClusterState currentState) {
+                    validateCompatibilityModeSettingRequest(request, state);
                     final ClusterState clusterState = updater.updateSettings(
                         currentState,
                         clusterSettings.upgradeSettings(request.transientSettings()),
@@ -279,42 +278,39 @@ public ClusterState execute(final ClusterState currentState) {
      * @param clusterState current state of cluster, for information on nodes
      */
     public static void validateCompatibilityModeSettingRequest(ClusterUpdateSettingsRequest request, ClusterState clusterState) {
-        if (RemoteStoreNodeService.REMOTE_STORE_COMPATIBILITY_MODE_SETTING.exists(request.persistentSettings())) {
-            String value = request.persistentSettings()
-                .get(RemoteStoreNodeService.REMOTE_STORE_COMPATIBILITY_MODE_SETTING.getKey())
-                .toLowerCase();
-            List<DiscoveryNode> discoveryNodeList = new ArrayList<>(clusterState.nodes().getNodes().values());
-            validateAllNodesOfSameVersion(discoveryNodeList);
+        Settings settings = Settings.builder().put(request.persistentSettings()).put(request.transientSettings()).build();
+        if (RemoteStoreNodeService.REMOTE_STORE_COMPATIBILITY_MODE_SETTING.exists(settings)) {
+            String value = settings.get(RemoteStoreNodeService.REMOTE_STORE_COMPATIBILITY_MODE_SETTING.getKey()).toLowerCase(Locale.ROOT);
+            validateAllNodesOfSameVersion(clusterState.nodes());
             if (value.equals(RemoteStoreNodeService.CompatibilityMode.STRICT.mode)) {
-                validateAllNodesOfSameType(discoveryNodeList);
+                validateAllNodesOfSameType(clusterState.nodes());
             }
         }
     }
 
     /**
      * Verifies that while trying to change the compatibility mode, all nodes must have the same version.
      * If not, it throws SettingsException error
-     * @param discoveryNodeList list of the current discovery nodes in the cluster
+     * @param discoveryNodes current discovery nodes in the cluster
      */
-    private static void validateAllNodesOfSameVersion(List<DiscoveryNode> discoveryNodeList) {
-        Set<Version> versions = discoveryNodeList.stream().map(DiscoveryNode::getVersion).collect(Collectors.toSet());
-        if (versions.size() != 1) {
-            throw new SettingsException(
-                "can not change the compatibility mode when all the nodes in cluster are not of the same version. Present versions: "
-                    + versions
-            );
+    private static void validateAllNodesOfSameVersion(DiscoveryNodes discoveryNodes) {
+        if (discoveryNodes.getMaxNodeVersion().equals(discoveryNodes.getMinNodeVersion()) == false) {
+            throw new SettingsException("can not change the compatibility mode when all the nodes in cluster are not of the same version");
         }
     }
 
     /**
      * Verifies that while trying to switch to STRICT compatibility mode, all nodes must be of the
      * same type (all remote or all non-remote). If not, it throws SettingsException error
-     * @param discoveryNodeList list of the current discovery nodes in the cluster
+     * @param discoveryNodes current discovery nodes in the cluster
      */
-    private static void validateAllNodesOfSameType(List<DiscoveryNode> discoveryNodeList) {
-        Optional<DiscoveryNode> remoteNode = discoveryNodeList.stream().filter(DiscoveryNode::isRemoteStoreNode).findFirst();
-        Optional<DiscoveryNode> nonRemoteNode = discoveryNodeList.stream().filter(dn -> dn.isRemoteStoreNode() == false).findFirst();
-        if (remoteNode.isPresent() && nonRemoteNode.isPresent()) {
+    private static void validateAllNodesOfSameType(DiscoveryNodes discoveryNodes) {
+        Set<Boolean> nodeTypes = discoveryNodes.getNodes()
+            .values()
+            .stream()
+            .map(DiscoveryNode::isRemoteStoreNode)
+            .collect(Collectors.toSet());
+        if (nodeTypes.size() != 1) {
             throw new SettingsException(
                 "can not switch to STRICT compatibility mode when the cluster contains both remote and non-remote nodes"
             );

diff --git a/.../org/opensearch/action/support/clustermanager/TransportClusterManagerNodeActionTests.java b/.../org/opensearch/action/support/clustermanager/TransportClusterManagerNodeActionTests.java
@@ -830,9 +830,7 @@ public void testDontAllowSwitchingCompatibilityModeForClusterWithMultipleVersion
         );
         assertThat(
             exception.getMessage(),
-            containsString(
-                "can not change the compatibility mode when all the nodes in cluster are not of the same version. Present versions: ["
-            )
+            containsString("can not change the compatibility mode when all the nodes in cluster are not of the same version")
         );
 
         // changing compatibility mode when all nodes are of the same version