From fe8d1b55998fac7780905d9b43e1f2f055716197 Mon Sep 17 00:00:00 2001 From: Lakshya Taragi Date: Fri, 12 Apr 2024 09:39:07 +0530 Subject: [PATCH] Address comments Signed-off-by: Lakshya Taragi --- .../RemoteStoreMigrationSettingsUpdateIT.java | 36 +++++++------- .../coordination/JoinTaskExecutorTests.java | 10 ++-- .../MetadataCreateIndexServiceTests.java | 47 +++++++++++-------- ...eStoreMigrationAllocationDeciderTests.java | 17 ++++--- 4 files changed, 62 insertions(+), 48 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationSettingsUpdateIT.java b/server/src/internalClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationSettingsUpdateIT.java index 478701095ecd0..de22b47af2dc6 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationSettingsUpdateIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotemigration/RemoteStoreMigrationSettingsUpdateIT.java @@ -39,46 +39,46 @@ public class RemoteStoreMigrationSettingsUpdateIT extends ShardAllocationBaseTes // remote store backed index setting tests public void testNewIndexIsRemoteStoreBackedForRemoteStoreDirectionAndMixedMode() { - logger.info(" --> initialize cluster: gives non remote cluster manager"); + logger.info("Initialize cluster: gives non remote cluster manager"); initializeCluster(false); String indexName1 = "test_index_1"; String indexName2 = "test_index_2"; - logger.info(" --> add non-remote node"); + logger.info("Add non-remote node"); addRemote = false; String nonRemoteNodeName = internalCluster().startNode(); internalCluster().validateClusterFormed(); assertNodeInCluster(nonRemoteNodeName); - logger.info(" --> create an index"); + logger.info("Create an index"); prepareIndexWithoutReplica(Optional.of(indexName1)); - logger.info(" --> verify that non remote-backed index is created"); + logger.info("Verify that non remote-backed index is created"); assertNonRemoteStoreBackedIndex(indexName1); - logger.info(" --> set mixed cluster compatibility mode and remote_store direction"); + logger.info("Set mixed cluster compatibility mode and remote_store direction"); setClusterMode(MIXED.mode); setDirection(REMOTE_STORE.direction); - logger.info(" --> add remote node"); + logger.info("Add remote node"); addRemote = true; String remoteNodeName = internalCluster().startNode(); internalCluster().validateClusterFormed(); assertNodeInCluster(remoteNodeName); - logger.info(" --> create another index"); + logger.info("Create another index"); prepareIndexWithoutReplica(Optional.of(indexName2)); - logger.info(" --> verify that remote backed index is created"); + logger.info("Verify that remote backed index is created"); assertRemoteStoreBackedIndex(indexName2); } public void testNewRestoredIndexIsRemoteStoreBackedForRemoteStoreDirectionAndMixedMode() throws Exception { - logger.info(" --> initialize cluster: gives non remote cluster manager"); + logger.info("Initialize cluster: gives non remote cluster manager"); initializeCluster(false); - logger.info(" --> add remote and non-remote nodes"); + logger.info("Add remote and non-remote nodes"); setClusterMode(MIXED.mode); addRemote = false; String nonRemoteNodeName = internalCluster().startNode(); @@ -88,7 +88,7 @@ public void testNewRestoredIndexIsRemoteStoreBackedForRemoteStoreDirectionAndMix assertNodeInCluster(nonRemoteNodeName); assertNodeInCluster(remoteNodeName); - logger.info(" --> create a non remote-backed index"); + logger.info("Create a non remote-backed index"); client.admin() .indices() .prepareCreate(TEST_INDEX) @@ -97,10 +97,10 @@ public void testNewRestoredIndexIsRemoteStoreBackedForRemoteStoreDirectionAndMix ) .get(); - logger.info(" --> verify that non remote stored backed index is created"); + logger.info("Verify that non remote stored backed index is created"); assertNonRemoteStoreBackedIndex(TEST_INDEX); - logger.info(" --> create repository"); + logger.info("Create repository"); String snapshotName = "test-snapshot"; String snapshotRepoName = "test-restore-snapshot-repo"; Path snapshotRepoNameAbsolutePath = randomRepoPath().toAbsolutePath(); @@ -110,7 +110,7 @@ public void testNewRestoredIndexIsRemoteStoreBackedForRemoteStoreDirectionAndMix .setSettings(Settings.builder().put("location", snapshotRepoNameAbsolutePath)) ); - logger.info(" --> create snapshot of non remote stored backed index"); + logger.info("Create snapshot of non remote stored backed index"); SnapshotInfo snapshotInfo = client().admin() .cluster() @@ -124,19 +124,19 @@ public void testNewRestoredIndexIsRemoteStoreBackedForRemoteStoreDirectionAndMix assertTrue(snapshotInfo.successfulShards() > 0); assertEquals(0, snapshotInfo.failedShards()); - logger.info(" --> restore index from snapshot under NONE direction"); + logger.info("Restore index from snapshot under NONE direction"); String restoredIndexName1 = TEST_INDEX + "-restored1"; restoreSnapshot(snapshotRepoName, snapshotName, restoredIndexName1); - logger.info(" --> verify that restored index is non remote-backed"); + logger.info("Verify that restored index is non remote-backed"); assertNonRemoteStoreBackedIndex(restoredIndexName1); - logger.info(" --> restore index from snapshot under REMOTE_STORE direction"); + logger.info("Restore index from snapshot under REMOTE_STORE direction"); setDirection(REMOTE_STORE.direction); String restoredIndexName2 = TEST_INDEX + "-restored2"; restoreSnapshot(snapshotRepoName, snapshotName, restoredIndexName2); - logger.info(" --> verify that restored index is non remote-backed"); + logger.info("Verify that restored index is non remote-backed"); assertRemoteStoreBackedIndex(restoredIndexName2); } diff --git a/server/src/test/java/org/opensearch/cluster/coordination/JoinTaskExecutorTests.java b/server/src/test/java/org/opensearch/cluster/coordination/JoinTaskExecutorTests.java index 875251183b802..5eafe63e63fad 100644 --- a/server/src/test/java/org/opensearch/cluster/coordination/JoinTaskExecutorTests.java +++ b/server/src/test/java/org/opensearch/cluster/coordination/JoinTaskExecutorTests.java @@ -922,16 +922,16 @@ private DiscoveryNode newDiscoveryNode(Map attributes) { ); } - public static final String SEGMENT_REPO = "segment-repo"; - public static final String TRANSLOG_REPO = "translog-repo"; + private static final String SEGMENT_REPO = "segment-repo"; + private static final String TRANSLOG_REPO = "translog-repo"; private static final String CLUSTER_STATE_REPO = "cluster-state-repo"; private static final String COMMON_REPO = "remote-repo"; - public static Map remoteStoreNodeAttributes(String segmentRepoName, String translogRepoName) { + private Map remoteStoreNodeAttributes(String segmentRepoName, String translogRepoName) { return remoteStoreNodeAttributes(segmentRepoName, translogRepoName, CLUSTER_STATE_REPO); } - private static Map remoteStoreNodeAttributes(String segmentRepoName, String translogRepoName, String clusterStateRepo) { + private Map remoteStoreNodeAttributes(String segmentRepoName, String translogRepoName, String clusterStateRepo) { String segmentRepositoryTypeAttributeKey = String.format( Locale.getDefault(), REMOTE_STORE_REPOSITORY_TYPE_ATTRIBUTE_KEY_FORMAT, @@ -968,7 +968,7 @@ private static Map remoteStoreNodeAttributes(String segmentRepoN }; } - private static Map remoteStateNodeAttributes(String clusterStateRepo) { + private Map remoteStateNodeAttributes(String clusterStateRepo) { String clusterStateRepositoryTypeAttributeKey = String.format( Locale.getDefault(), REMOTE_STORE_REPOSITORY_TYPE_ATTRIBUTE_KEY_FORMAT, 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 9f7fe419a63e5..74271a1b5bf61 100644 --- a/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java +++ b/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java @@ -121,8 +121,6 @@ import static java.util.Collections.emptyMap; import static java.util.Collections.singleton; import static java.util.Collections.singletonList; -import static org.opensearch.cluster.coordination.JoinTaskExecutorTests.SEGMENT_REPO; -import static org.opensearch.cluster.coordination.JoinTaskExecutorTests.TRANSLOG_REPO; import static org.opensearch.cluster.metadata.IndexMetadata.INDEX_NUMBER_OF_ROUTING_SHARDS_SETTING; import static org.opensearch.cluster.metadata.IndexMetadata.INDEX_NUMBER_OF_SHARDS_SETTING; import static org.opensearch.cluster.metadata.IndexMetadata.INDEX_READ_ONLY_BLOCK; @@ -141,8 +139,6 @@ import static org.opensearch.cluster.metadata.MetadataCreateIndexService.getIndexNumberOfRoutingShards; import static org.opensearch.cluster.metadata.MetadataCreateIndexService.parseV1Mappings; import static org.opensearch.cluster.metadata.MetadataCreateIndexService.resolveAndValidateAliases; -import static org.opensearch.cluster.routing.allocation.RemoteStoreMigrationAllocationDeciderTests.getNonRemoteNode; -import static org.opensearch.cluster.routing.allocation.RemoteStoreMigrationAllocationDeciderTests.getRemoteNode; import static org.opensearch.common.util.FeatureFlags.REMOTE_STORE_MIGRATION_EXPERIMENTAL; import static org.opensearch.index.IndexModule.INDEX_STORE_TYPE_SETTING; import static org.opensearch.index.IndexSettings.INDEX_REFRESH_INTERVAL_SETTING; @@ -1364,8 +1360,8 @@ public void testRemoteStoreNoUserOverrideExceptReplicationTypeSegmentIndexSettin .build(); Settings settings = Settings.builder() .put(CLUSTER_REPLICATION_TYPE_SETTING.getKey(), ReplicationType.DOCUMENT) - .put(segmentRepositoryNameAttributeKey, SEGMENT_REPO) - .put(translogRepositoryNameAttributeKey, TRANSLOG_REPO) + .put(segmentRepositoryNameAttributeKey, "my-segment-repo-1") + .put(translogRepositoryNameAttributeKey, "my-translog-repo-1") .build(); request = new CreateIndexClusterStateUpdateRequest("create index", "test", "test"); final Settings.Builder requestSettings = Settings.builder(); @@ -1385,8 +1381,8 @@ public void testRemoteStoreNoUserOverrideExceptReplicationTypeSegmentIndexSettin verifyRemoteStoreIndexSettings( indexSettings, "true", - SEGMENT_REPO, - TRANSLOG_REPO, + "my-segment-repo-1", + "my-translog-repo-1", ReplicationType.SEGMENT.toString(), IndexSettings.DEFAULT_REMOTE_TRANSLOG_BUFFER_INTERVAL ); @@ -1397,8 +1393,8 @@ public void testRemoteStoreImplicitOverrideReplicationTypeToSegmentForRemoteStor .nodes(DiscoveryNodes.builder().add(getRemoteNode()).build()) .build(); Settings settings = Settings.builder() - .put(segmentRepositoryNameAttributeKey, SEGMENT_REPO) - .put(translogRepositoryNameAttributeKey, TRANSLOG_REPO) + .put(segmentRepositoryNameAttributeKey, "my-segment-repo-1") + .put(translogRepositoryNameAttributeKey, "my-translog-repo-1") .build(); request = new CreateIndexClusterStateUpdateRequest("create index", "test", "test"); final Settings.Builder requestSettings = Settings.builder(); @@ -1417,8 +1413,8 @@ public void testRemoteStoreImplicitOverrideReplicationTypeToSegmentForRemoteStor verifyRemoteStoreIndexSettings( indexSettings, "true", - SEGMENT_REPO, - TRANSLOG_REPO, + "my-segment-repo-1", + "my-translog-repo-1", ReplicationType.SEGMENT.toString(), IndexSettings.DEFAULT_REMOTE_TRANSLOG_BUFFER_INTERVAL ); @@ -1430,8 +1426,8 @@ public void testRemoteStoreNoUserOverrideIndexSettings() { .build(); Settings settings = Settings.builder() .put(CLUSTER_REPLICATION_TYPE_SETTING.getKey(), ReplicationType.SEGMENT) - .put(segmentRepositoryNameAttributeKey, SEGMENT_REPO) - .put(translogRepositoryNameAttributeKey, TRANSLOG_REPO) + .put(segmentRepositoryNameAttributeKey, "my-segment-repo-1") + .put(translogRepositoryNameAttributeKey, "my-translog-repo-1") .build(); request = new CreateIndexClusterStateUpdateRequest("create index", "test", "test"); Settings indexSettings = aggregateIndexSettings( @@ -1448,8 +1444,8 @@ public void testRemoteStoreNoUserOverrideIndexSettings() { verifyRemoteStoreIndexSettings( indexSettings, "true", - SEGMENT_REPO, - TRANSLOG_REPO, + "my-segment-repo-1", + "my-translog-repo-1", ReplicationType.SEGMENT.toString(), IndexSettings.DEFAULT_REMOTE_TRANSLOG_BUFFER_INTERVAL ); @@ -1571,7 +1567,7 @@ public void testNewIndexIsRemoteStoreBackedForRemoteStoreDirectionAndMixedMode() FeatureFlags.initializeFeatureFlags(Settings.builder().put(REMOTE_STORE_MIGRATION_EXPERIMENTAL, "true").build()); // non-remote cluster manager node - DiscoveryNode nonRemoteClusterManagerNode = getNonRemoteNode(); + DiscoveryNode nonRemoteClusterManagerNode = new DiscoveryNode(UUIDs.base64UUID(), buildNewFakeTransportAddress(), Version.CURRENT); DiscoveryNodes discoveryNodes = DiscoveryNodes.builder() .add(nonRemoteClusterManagerNode) @@ -1634,8 +1630,8 @@ public void testNewIndexIsRemoteStoreBackedForRemoteStoreDirectionAndMixedMode() verifyRemoteStoreIndexSettings( indexSettings, "true", - SEGMENT_REPO, - TRANSLOG_REPO, + "my-segment-repo-1", + "my-translog-repo-1", ReplicationType.SEGMENT.toString(), IndexSettings.DEFAULT_REMOTE_TRANSLOG_BUFFER_INTERVAL ); @@ -2248,6 +2244,19 @@ private void verifyRemoteStoreIndexSettings( assertEquals(translogBufferInterval, INDEX_REMOTE_TRANSLOG_BUFFER_INTERVAL_SETTING.get(indexSettings)); } + private DiscoveryNode getRemoteNode() { + Map attributes = new HashMap<>(); + attributes.put(REMOTE_STORE_SEGMENT_REPOSITORY_NAME_ATTRIBUTE_KEY, "my-segment-repo-1"); + attributes.put(REMOTE_STORE_TRANSLOG_REPOSITORY_NAME_ATTRIBUTE_KEY, "my-translog-repo-1"); + return new DiscoveryNode( + UUIDs.base64UUID(), + buildNewFakeTransportAddress(), + attributes, + DiscoveryNodeRole.BUILT_IN_ROLES, + Version.CURRENT + ); + } + @After public void shutdown() throws Exception { clusterSettings = null; diff --git a/server/src/test/java/org/opensearch/cluster/routing/allocation/RemoteStoreMigrationAllocationDeciderTests.java b/server/src/test/java/org/opensearch/cluster/routing/allocation/RemoteStoreMigrationAllocationDeciderTests.java index 808a548cceeb0..3e130a42952e4 100644 --- a/server/src/test/java/org/opensearch/cluster/routing/allocation/RemoteStoreMigrationAllocationDeciderTests.java +++ b/server/src/test/java/org/opensearch/cluster/routing/allocation/RemoteStoreMigrationAllocationDeciderTests.java @@ -60,16 +60,16 @@ import org.opensearch.node.remotestore.RemoteStoreNodeService; import java.util.Collections; +import java.util.HashMap; import java.util.Locale; +import java.util.Map; -import static org.opensearch.cluster.coordination.JoinTaskExecutorTests.SEGMENT_REPO; -import static org.opensearch.cluster.coordination.JoinTaskExecutorTests.TRANSLOG_REPO; -import static org.opensearch.cluster.coordination.JoinTaskExecutorTests.remoteStoreNodeAttributes; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_SEGMENT_STORE_REPOSITORY; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_STORE_ENABLED; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REMOTE_TRANSLOG_STORE_REPOSITORY; import static org.opensearch.cluster.metadata.IndexMetadata.SETTING_REPLICATION_TYPE; import static org.opensearch.common.util.FeatureFlags.REMOTE_STORE_MIGRATION_EXPERIMENTAL; +import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_CLUSTER_STATE_REPOSITORY_NAME_ATTRIBUTE_KEY; import static org.opensearch.node.remotestore.RemoteStoreNodeService.MIGRATION_DIRECTION_SETTING; import static org.opensearch.node.remotestore.RemoteStoreNodeService.REMOTE_STORE_COMPATIBILITY_MODE_SETTING; import static org.hamcrest.core.Is.is; @@ -659,16 +659,21 @@ private ClusterState getInitialClusterState( } // get a dummy non-remote node - public static DiscoveryNode getNonRemoteNode() { + private DiscoveryNode getNonRemoteNode() { return new DiscoveryNode(UUIDs.base64UUID(), buildNewFakeTransportAddress(), Version.CURRENT); } // get a dummy remote node - public static DiscoveryNode getRemoteNode() { + private DiscoveryNode getRemoteNode() { + Map attributes = new HashMap<>(); + attributes.put( + REMOTE_STORE_CLUSTER_STATE_REPOSITORY_NAME_ATTRIBUTE_KEY, + "REMOTE_STORE_CLUSTER_STATE_REPOSITORY_NAME_ATTRIBUTE_VALUE" + ); return new DiscoveryNode( UUIDs.base64UUID(), buildNewFakeTransportAddress(), - remoteStoreNodeAttributes(SEGMENT_REPO, TRANSLOG_REPO), + attributes, DiscoveryNodeRole.BUILT_IN_ROLES, Version.CURRENT );