From 3a1afc001758c5687c3f84219825eda95531a1a1 Mon Sep 17 00:00:00 2001 From: Arpit Bandejiya Date: Thu, 5 Jan 2023 16:25:21 +0530 Subject: [PATCH] Addressing comments Signed-off-by: Arpit Bandejiya --- CHANGELOG.md | 2 +- .../opensearch/cluster/metadata/Metadata.java | 2 +- .../TransportReplicationActionTests.java | 7 +++ .../MetadataCreateIndexServiceTests.java | 40 ++++---------- .../AwarenessReplicaBalanceTests.java | 53 ------------------- .../test/OpenSearchIntegTestCase.java | 4 +- 6 files changed, 20 insertions(+), 88 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f71c7ec96897..309802bfa48c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ## [Unreleased 3.0] ### Added -- Add support of default replica count change using total awareness attribute([#5610](https://github.com/opensearch-project/OpenSearch/pull/5610)) +- Add support of default replica count cluster setting ([#5610](https://github.com/opensearch-project/OpenSearch/pull/5610)) - Hardened token permissions in GitHub workflows ([#4587](https://github.com/opensearch-project/OpenSearch/pull/4587)) - Support for HTTP/2 (server-side) ([#3847](https://github.com/opensearch-project/OpenSearch/pull/3847)) - Add getter for path field in NestedQueryBuilder ([#4636](https://github.com/opensearch-project/OpenSearch/pull/4636)) diff --git a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java index 201e9042dcfb5..f5fd32802f40e 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/Metadata.java @@ -162,7 +162,7 @@ public interface Custom extends NamedDiffable, ToXContentFragment, Clust } public static final Setting DEFAULT_REPLICA_COUNT_SETTING = Setting.intSetting( - "cluster.default_replica_count", + "cluster.default_number_of_replicas", 1, Property.Dynamic, Property.NodeScope diff --git a/server/src/test/java/org/opensearch/action/support/replication/TransportReplicationActionTests.java b/server/src/test/java/org/opensearch/action/support/replication/TransportReplicationActionTests.java index bde483e171d1e..285dc13c32abf 100644 --- a/server/src/test/java/org/opensearch/action/support/replication/TransportReplicationActionTests.java +++ b/server/src/test/java/org/opensearch/action/support/replication/TransportReplicationActionTests.java @@ -201,6 +201,13 @@ public void setUp() throws Exception { transportService.acceptIncomingRequests(); shardStateAction = new ShardStateAction(clusterService, transportService, null, null, threadPool); action = new TestAction(Settings.EMPTY, "internal:testAction", transportService, clusterService, shardStateAction, threadPool); + Metadata metadata = Metadata.builder() + .transientSettings(Settings.builder().put(Metadata.DEFAULT_REPLICA_COUNT_SETTING.getKey(), 1).build()) + .build(); + ClusterState clusterState = ClusterState.builder(org.opensearch.cluster.ClusterName.CLUSTER_NAME_SETTING.getDefault(Settings.EMPTY)) + .metadata(metadata) + .build(); + setState(clusterService, clusterState); } @Override diff --git a/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java b/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java index 86a9f725ba5b6..4e58fd80c0f3b 100644 --- a/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java +++ b/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java @@ -1072,6 +1072,13 @@ public void testParseMappingsWithTypelessTemplate() throws Exception { public void testvalidateIndexSettings() { ClusterService clusterService = mock(ClusterService.class); + Metadata metadata = Metadata.builder() + .transientSettings(Settings.builder().put(Metadata.DEFAULT_REPLICA_COUNT_SETTING.getKey(), 1).build()) + .build(); + ClusterState clusterState = ClusterState.builder(org.opensearch.cluster.ClusterName.CLUSTER_NAME_SETTING.getDefault(Settings.EMPTY)) + .metadata(metadata) + .build(); + ThreadPool threadPool = new TestThreadPool(getTestName()); Settings settings = Settings.builder() .put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_ATTRIBUTE_SETTING.getKey(), "zone, rack") @@ -1082,6 +1089,8 @@ public void testvalidateIndexSettings() { ClusterSettings clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); when(clusterService.getSettings()).thenReturn(settings); when(clusterService.getClusterSettings()).thenReturn(clusterSettings); + when(clusterService.state()).thenReturn(clusterState); + MetadataCreateIndexService checkerService = new MetadataCreateIndexService( settings, clusterService, @@ -1113,37 +1122,6 @@ public void testvalidateIndexSettings() { validationErrors = checkerService.getIndexSettingsValidationErrors(settings, false, Optional.empty()); assertThat(validationErrors.size(), is(0)); - settings = Settings.builder() - .put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_ATTRIBUTE_SETTING.getKey(), "zone, rack") - .put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_FORCE_GROUP_SETTING.getKey() + "zone.values", "a, b") - .put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_FORCE_GROUP_SETTING.getKey() + "rack.values", "c, d, e") - .put(AwarenessReplicaBalance.CLUSTER_ROUTING_ALLOCATION_AWARENESS_BALANCE_SETTING.getKey(), true) - .put(AwarenessReplicaBalance.FORCE_AWARENESS_REPLICA_SETTING.getKey(), true) - .build(); - - clusterSettings = new ClusterSettings(settings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); - when(clusterService.getSettings()).thenReturn(settings); - when(clusterService.getClusterSettings()).thenReturn(clusterSettings); - - checkerService = new MetadataCreateIndexService( - settings, - clusterService, - null, - null, - null, - createTestShardLimitService(randomIntBetween(1, 1000), false, clusterService), - new Environment(Settings.builder().put("path.home", "dummy").build(), null), - IndexScopedSettings.DEFAULT_SCOPED_SETTINGS, - threadPool, - null, - new SystemIndices(Collections.emptyMap()), - true, - new AwarenessReplicaBalance(settings, clusterService.getClusterSettings()) - ); - - validationErrors = checkerService.getIndexSettingsValidationErrors(settings, false, Optional.empty()); - assertThat(validationErrors.size(), is(0)); - threadPool.shutdown(); } diff --git a/server/src/test/java/org/opensearch/cluster/routing/allocation/AwarenessReplicaBalanceTests.java b/server/src/test/java/org/opensearch/cluster/routing/allocation/AwarenessReplicaBalanceTests.java index 098741ab3c904..f7b1b8694f91a 100644 --- a/server/src/test/java/org/opensearch/cluster/routing/allocation/AwarenessReplicaBalanceTests.java +++ b/server/src/test/java/org/opensearch/cluster/routing/allocation/AwarenessReplicaBalanceTests.java @@ -39,30 +39,6 @@ public void testNoForcedAwarenessAttribute() { assertEquals(awarenessReplicaBalance.validate(1, autoExpandReplica), Optional.empty()); } - public void testStaticMaxAwarenessAttributes() { - Settings settings = Settings.builder() - .put("cluster.routing.allocation.awareness.attributes", "rack_id") - .put(SETTING_AUTO_EXPAND_REPLICAS, "0-1") - .build(); - assertThat(AwarenessReplicaBalance.maxAwarenessAttributes(settings), equalTo(1)); - - settings = Settings.builder() - .put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_ATTRIBUTE_SETTING.getKey(), "zone, rack") - .put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_FORCE_GROUP_SETTING.getKey() + "zone.values", "a, b") - .put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_FORCE_GROUP_SETTING.getKey() + "rack.values", "c, d, e") - .put(AwarenessReplicaBalance.CLUSTER_ROUTING_ALLOCATION_AWARENESS_BALANCE_SETTING.getKey(), true) - .put(SETTING_AUTO_EXPAND_REPLICAS, "0-2") - .build(); - assertThat(AwarenessReplicaBalance.maxAwarenessAttributes(settings), equalTo(3)); - - settings = Settings.builder() - .put(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_ATTRIBUTE_SETTING.getKey(), "zone, rack") - .put(AwarenessReplicaBalance.CLUSTER_ROUTING_ALLOCATION_AWARENESS_BALANCE_SETTING.getKey(), true) - .put(SETTING_AUTO_EXPAND_REPLICAS, "0-1") - .build(); - assertThat(AwarenessReplicaBalance.maxAwarenessAttributes(settings), equalTo(1)); - } - public void testForcedAwarenessAttribute() { // When auto expand replica settings is as per zone awareness Settings settings = Settings.builder() @@ -155,33 +131,4 @@ public void testForcedAwarenessAttributeDisabled() { assertEquals(awarenessReplicaBalance.validate(1, autoExpandReplica), Optional.empty()); } - public void testSetUseForceZoneForReplicaWhenAllocationAwarenessIsDefault() { - Settings settings = Settings.builder().put(AwarenessReplicaBalance.FORCE_AWARENESS_REPLICA_SETTING.getKey(), true).build(); - - IllegalArgumentException iae = expectThrows( - IllegalArgumentException.class, - () -> new AwarenessReplicaBalance(settings, EMPTY_CLUSTER_SETTINGS) - ); - assertEquals( - "To enable cluster.routing.allocation.awareness.force_replica, cluster.routing.allocation.awareness.balance should be enabled", - iae.getMessage() - ); - } - - public void testSetUseForceZoneForReplicaWhenAllocationAwarenessIsFalse() { - Settings settings = Settings.builder() - .put(AwarenessReplicaBalance.CLUSTER_ROUTING_ALLOCATION_AWARENESS_BALANCE_SETTING.getKey(), false) - .put(AwarenessReplicaBalance.FORCE_AWARENESS_REPLICA_SETTING.getKey(), true) - .build(); - - IllegalArgumentException iae = expectThrows( - IllegalArgumentException.class, - () -> new AwarenessReplicaBalance(settings, EMPTY_CLUSTER_SETTINGS) - ); - assertEquals( - "To enable cluster.routing.allocation.awareness.force_replica, cluster.routing.allocation.awareness.balance should be enabled", - iae.getMessage() - ); - } - } diff --git a/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java b/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java index 38f5813247e2e..ba475e855266d 100644 --- a/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java +++ b/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java @@ -2413,14 +2413,14 @@ public void manageUseZoneForReplicaSetting(boolean apply) { "a, b, c" ) .put(AwarenessReplicaBalance.CLUSTER_ROUTING_ALLOCATION_AWARENESS_BALANCE_SETTING.getKey(), true) - .put(AwarenessReplicaBalance.FORCE_AWARENESS_REPLICA_SETTING.getKey(), true) + .put(Metadata.DEFAULT_REPLICA_COUNT_SETTING.getKey(), 2) .build(); } else { settings = Settings.builder() .putNull(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_ATTRIBUTE_SETTING.getKey()) .putNull(AwarenessAllocationDecider.CLUSTER_ROUTING_ALLOCATION_AWARENESS_FORCE_GROUP_SETTING.getKey() + "zone.values") .putNull(AwarenessReplicaBalance.CLUSTER_ROUTING_ALLOCATION_AWARENESS_BALANCE_SETTING.getKey()) - .putNull(AwarenessReplicaBalance.FORCE_AWARENESS_REPLICA_SETTING.getKey()) + .putNull(Metadata.DEFAULT_REPLICA_COUNT_SETTING.getKey()) .build(); } ClusterUpdateSettingsRequest updateSettingsRequest = new ClusterUpdateSettingsRequest();