From 6d434aabf866995c6cda95491a790e13e2759bf2 Mon Sep 17 00:00:00 2001 From: Lakshya Taragi Date: Mon, 8 Apr 2024 19:14:08 +0530 Subject: [PATCH] Address comments Signed-off-by: Lakshya Taragi --- .../RemoteStoreMigrationAllocationIT.java | 14 +++++- .../TransportClusterUpdateSettingsAction.java | 46 +++++++++---------- ...ransportClusterManagerNodeActionTests.java | 4 +- 3 files changed, 34 insertions(+), 30 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationAllocationIT.java b/server/src/internalClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationAllocationIT.java index dd45c521247b4..bc9bd25b60255 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationAllocationIT.java +++ b/server/src/internalClusterTest/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/server/src/main/java/org/opensearch/action/admin/cluster/settings/TransportClusterUpdateSettingsAction.java b/server/src/main/java/org/opensearch/action/admin/cluster/settings/TransportClusterUpdateSettingsAction.java index a2d3a7de6a5c6..2d2e5ca696c00 100644 --- a/server/src/main/java/org/opensearch/action/admin/cluster/settings/TransportClusterUpdateSettingsAction.java +++ b/server/src/main/java/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 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,14 +278,12 @@ 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 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()); } } } @@ -294,27 +291,26 @@ public static void validateCompatibilityModeSettingRequest(ClusterUpdateSettings /** * 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 discoveryNodeList) { - Set 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 discoveryNodeList) { - Optional remoteNode = discoveryNodeList.stream().filter(DiscoveryNode::isRemoteStoreNode).findFirst(); - Optional nonRemoteNode = discoveryNodeList.stream().filter(dn -> dn.isRemoteStoreNode() == false).findFirst(); - if (remoteNode.isPresent() && nonRemoteNode.isPresent()) { + private static void validateAllNodesOfSameType(DiscoveryNodes discoveryNodes) { + Set 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/server/src/test/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeActionTests.java b/server/src/test/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeActionTests.java index 2e669b911f6c6..505dcd68fe6d7 100644 --- a/server/src/test/java/org/opensearch/action/support/clustermanager/TransportClusterManagerNodeActionTests.java +++ b/server/src/test/java/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