From d256b66e3dd85a9b4c02a3f4a8b6951b86cd8e52 Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Tue, 12 Mar 2024 12:35:16 +0530 Subject: [PATCH 1/3] Introduce remote store path type in customData in IndexMetadata Signed-off-by: Ashish Singh --- .../cluster/metadata/IndexMetadata.java | 1 + .../metadata/MetadataCreateIndexService.java | 34 +++++-- .../common/settings/ClusterSettings.java | 1 + .../index/remote/RemoteStorePathType.java | 37 ++++++++ .../opensearch/indices/IndicesService.java | 11 +++ .../MetadataCreateIndexServiceTests.java | 94 ++++++++++++++++++- 6 files changed, 169 insertions(+), 9 deletions(-) create mode 100644 server/src/main/java/org/opensearch/index/remote/RemoteStorePathType.java diff --git a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java index 03784df509ed6..80b78cfe154f1 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java @@ -635,6 +635,7 @@ public static APIBlock readFrom(StreamInput input) throws IOException { static final String KEY_ROLLOVER_INFOS = "rollover_info"; static final String KEY_SYSTEM = "system"; public static final String KEY_PRIMARY_TERMS = "primary_terms"; + public static final String REMOTE_STORE_CUSTOM_KEY = "remote_store"; public static final String INDEX_STATE_FILE_PREFIX = "state-"; diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java index 4dde5d0ea013f..34405c0647676 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java @@ -88,6 +88,7 @@ import org.opensearch.index.mapper.MapperService; import org.opensearch.index.mapper.MapperService.MergeReason; import org.opensearch.index.query.QueryShardContext; +import org.opensearch.index.remote.RemoteStorePathType; import org.opensearch.index.shard.IndexSettingProvider; import org.opensearch.index.translog.Translog; import org.opensearch.indices.IndexCreationException; @@ -498,7 +499,8 @@ private ClusterState applyCreateIndexWithTemporaryService( temporaryIndexMeta.getSettings(), temporaryIndexMeta.getRoutingNumShards(), sourceMetadata, - temporaryIndexMeta.isSystem() + temporaryIndexMeta.isSystem(), + temporaryIndexMeta.getCustomData() ); } catch (Exception e) { logger.info("failed to build index metadata [{}]", request.index()); @@ -522,10 +524,11 @@ private ClusterState applyCreateIndexWithTemporaryService( /** * Given a state and index settings calculated after applying templates, validate metadata for - * the new index, returning an {@link IndexMetadata} for the new index + * the new index, returning an {@link IndexMetadata} for the new index. + *

+ * The access level of the method changed to default level for visibility to test. */ - private IndexMetadata buildAndValidateTemporaryIndexMetadata( - final ClusterState currentState, + IndexMetadata buildAndValidateTemporaryIndexMetadata( final Settings aggregatedIndexSettings, final CreateIndexClusterStateUpdateRequest request, final int routingNumShards @@ -544,6 +547,16 @@ private IndexMetadata buildAndValidateTemporaryIndexMetadata( tmpImdBuilder.settings(indexSettings); tmpImdBuilder.system(isSystem); + if (isRemoteStoreAttributePresent(settings)) { + String pathType; + if (clusterService.getClusterSettings().get(IndicesService.CLUSTER_REMOTE_STORE_PATH_PREFIX_OPTIMISED_SETTING)) { + pathType = RemoteStorePathType.HASHED_PREFIX.toString(); + } else { + pathType = RemoteStorePathType.FIXED.toString(); + } + tmpImdBuilder.putCustom(IndexMetadata.REMOTE_STORE_CUSTOM_KEY, Map.of(RemoteStorePathType.NAME, pathType)); + } + // Set up everything, now locally create the index to see that things are ok, and apply IndexMetadata tempMetadata = tmpImdBuilder.build(); validateActiveShardCount(request.waitForActiveShards(), tempMetadata); @@ -582,7 +595,7 @@ private ClusterState applyCreateIndexRequestWithV1Templates( clusterService.getClusterSettings() ); int routingNumShards = getIndexNumberOfRoutingShards(aggregatedIndexSettings, null); - IndexMetadata tmpImd = buildAndValidateTemporaryIndexMetadata(currentState, aggregatedIndexSettings, request, routingNumShards); + IndexMetadata tmpImd = buildAndValidateTemporaryIndexMetadata(aggregatedIndexSettings, request, routingNumShards); return applyCreateIndexWithTemporaryService( currentState, @@ -647,7 +660,7 @@ private ClusterState applyCreateIndexRequestWithV2Template( clusterService.getClusterSettings() ); int routingNumShards = getIndexNumberOfRoutingShards(aggregatedIndexSettings, null); - IndexMetadata tmpImd = buildAndValidateTemporaryIndexMetadata(currentState, aggregatedIndexSettings, request, routingNumShards); + IndexMetadata tmpImd = buildAndValidateTemporaryIndexMetadata(aggregatedIndexSettings, request, routingNumShards); return applyCreateIndexWithTemporaryService( currentState, @@ -728,7 +741,7 @@ private ClusterState applyCreateIndexRequestWithExistingMetadata( clusterService.getClusterSettings() ); final int routingNumShards = getIndexNumberOfRoutingShards(aggregatedIndexSettings, sourceMetadata); - IndexMetadata tmpImd = buildAndValidateTemporaryIndexMetadata(currentState, aggregatedIndexSettings, request, routingNumShards); + IndexMetadata tmpImd = buildAndValidateTemporaryIndexMetadata(aggregatedIndexSettings, request, routingNumShards); return applyCreateIndexWithTemporaryService( currentState, @@ -1147,7 +1160,8 @@ static IndexMetadata buildIndexMetadata( Settings indexSettings, int routingNumShards, @Nullable IndexMetadata sourceMetadata, - boolean isSystem + boolean isSystem, + Map customData ) { IndexMetadata.Builder indexMetadataBuilder = createIndexMetadataBuilder(indexName, sourceMetadata, indexSettings, routingNumShards); indexMetadataBuilder.system(isSystem); @@ -1168,6 +1182,10 @@ static IndexMetadata buildIndexMetadata( indexMetadataBuilder.putAlias(aliases.get(i)); } + for (Map.Entry entry : customData.entrySet()) { + indexMetadataBuilder.putCustom(entry.getKey(), entry.getValue()); + } + indexMetadataBuilder.state(IndexMetadata.State.OPEN); return indexMetadataBuilder.build(); } diff --git a/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java b/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java index 5090010198a5d..f80c6cdbb95ce 100644 --- a/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java @@ -709,6 +709,7 @@ public void apply(Settings value, Settings current, Settings previous) { CpuBasedAdmissionControllerSettings.INDEXING_CPU_USAGE_LIMIT, CpuBasedAdmissionControllerSettings.SEARCH_CPU_USAGE_LIMIT, IndicesService.CLUSTER_INDEX_RESTRICT_REPLICATION_TYPE_SETTING, + IndicesService.CLUSTER_REMOTE_STORE_PATH_PREFIX_OPTIMISED_SETTING, // Concurrent segment search settings SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING, diff --git a/server/src/main/java/org/opensearch/index/remote/RemoteStorePathType.java b/server/src/main/java/org/opensearch/index/remote/RemoteStorePathType.java new file mode 100644 index 0000000000000..5547d84c1251c --- /dev/null +++ b/server/src/main/java/org/opensearch/index/remote/RemoteStorePathType.java @@ -0,0 +1,37 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.remote; + +/** + * Enumerates the types of remote store paths resolution techniques supported by OpenSearch. + * For more information, see Github issue #12567. + * + * @opensearch.internal + */ +public enum RemoteStorePathType { + + FIXED, + HASHED_PREFIX; + + public static RemoteStorePathType parseString(String remoteStorePathType) { + try { + return RemoteStorePathType.valueOf(remoteStorePathType); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException("Could not parse RemoteStorePathType for [" + remoteStorePathType + "]"); + } catch (NullPointerException npe) { + // return a default value for null input + return FIXED; + } + } + + /** + * This string is used as key for storing information in the custom data in index settings. + */ + public static final String NAME = "path_type"; +} diff --git a/server/src/main/java/org/opensearch/indices/IndicesService.java b/server/src/main/java/org/opensearch/indices/IndicesService.java index 8151c151e3968..b56a84df3e80c 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesService.java +++ b/server/src/main/java/org/opensearch/indices/IndicesService.java @@ -314,6 +314,17 @@ public class IndicesService extends AbstractLifecycleComponent Property.Final ); + /** + * This setting is used to enable the optimisation in prefix path which helps in achieving higher throughput and lesser + * rate limiting by remote store providers. This setting is effective only for remote store enabled cluster. + */ + public static final Setting CLUSTER_REMOTE_STORE_PATH_PREFIX_OPTIMISED_SETTING = Setting.boolSetting( + "cluster.remote_store.index.path.prefix.optimised", + false, + Property.NodeScope, + Property.Dynamic + ); + /** * The node's settings. */ 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 6d1f359d210ac..c5924f1de5bdd 100644 --- a/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java +++ b/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java @@ -71,6 +71,7 @@ import org.opensearch.index.IndexSettings; import org.opensearch.index.mapper.MapperService; import org.opensearch.index.query.QueryShardContext; +import org.opensearch.index.remote.RemoteStorePathType; import org.opensearch.index.translog.Translog; import org.opensearch.indices.IndexCreationException; import org.opensearch.indices.IndicesService; @@ -1563,13 +1564,104 @@ public void testBuildIndexMetadata() { .put(SETTING_NUMBER_OF_SHARDS, 1) .build(); List aliases = singletonList(AliasMetadata.builder("alias1").build()); - IndexMetadata indexMetadata = buildIndexMetadata("test", aliases, () -> null, indexSettings, 4, sourceIndexMetadata, false); + IndexMetadata indexMetadata = buildIndexMetadata( + "test", + aliases, + () -> null, + indexSettings, + 4, + sourceIndexMetadata, + false, + new HashMap<>() + ); assertThat(indexMetadata.getAliases().size(), is(1)); assertThat(indexMetadata.getAliases().keySet().iterator().next(), is("alias1")); assertThat("The source index primary term must be used", indexMetadata.primaryTerm(0), is(3L)); } + /** + * This test checks if the cluster is a remote store cluster then we populate custom data for remote settings in + * index metadata of the underlying index. This captures information around the resolution pattern of the path for + * remote segments and translog. + */ + public void testRemoteCustomData() { + // Case 1 - Remote store is not enabled + IndexMetadata indexMetadata = testRemoteCustomData(false, randomBoolean()); + assertNull(indexMetadata.getCustomData(IndexMetadata.REMOTE_STORE_CUSTOM_KEY)); + + // Case 2 - cluster.remote_store.index.path.prefix.optimised=false (default value) + indexMetadata = testRemoteCustomData(true, false); + validateRemoteCustomData( + indexMetadata.getCustomData(IndexMetadata.REMOTE_STORE_CUSTOM_KEY), + RemoteStorePathType.NAME, + RemoteStorePathType.FIXED.toString() + ); + + // Case 3 - cluster.remote_store.index.path.prefix.optimised=true + indexMetadata = testRemoteCustomData(true, true); + validateRemoteCustomData( + indexMetadata.getCustomData(IndexMetadata.REMOTE_STORE_CUSTOM_KEY), + RemoteStorePathType.NAME, + RemoteStorePathType.HASHED_PREFIX.toString() + ); + } + + private IndexMetadata testRemoteCustomData(boolean remoteStoreEnabled, boolean optimisedPrefix) { + Settings.Builder settingsBuilder = Settings.builder(); + if (remoteStoreEnabled) { + settingsBuilder.put(NODE_ATTRIBUTES.getKey() + REMOTE_STORE_SEGMENT_REPOSITORY_NAME_ATTRIBUTE_KEY, "test"); + } + if (optimisedPrefix) { + settingsBuilder.put(IndicesService.CLUSTER_REMOTE_STORE_PATH_PREFIX_OPTIMISED_SETTING.getKey(), true); + } + Settings settings = settingsBuilder.build(); + + 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(); + 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); + + ThreadPool threadPool = new TestThreadPool(getTestName()); + MetadataCreateIndexService metadataCreateIndexService = 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()) + ); + CreateIndexClusterStateUpdateRequest request = new CreateIndexClusterStateUpdateRequest("create index", "test", "test"); + Settings indexSettings = Settings.builder() + .put("index.version.created", Version.CURRENT) + .put(INDEX_NUMBER_OF_SHARDS_SETTING.getKey(), 3) + .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 1) + .build(); + + IndexMetadata indexMetadata = metadataCreateIndexService.buildAndValidateTemporaryIndexMetadata(indexSettings, request, 0); + threadPool.shutdown(); + return indexMetadata; + } + + private void validateRemoteCustomData(Map customData, String expectedKey, String expectedValue) { + assertTrue(customData.containsKey(expectedKey)); + assertEquals(expectedValue, customData.get(expectedKey)); + } + public void testGetIndexNumberOfRoutingShardsWithNullSourceIndex() { Settings indexSettings = Settings.builder() .put("index.version.created", Version.CURRENT) From 8ab4d9eac99f9cd5579e29570100736644f42c48 Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Fri, 15 Mar 2024 11:20:20 +0530 Subject: [PATCH 2/3] Incorporate PR review comments Signed-off-by: Ashish Singh --- .../metadata/MetadataCreateIndexService.java | 20 ++++++------ .../remote/RemoteStoreBlobPathResolver.java | 32 +++++++++++++++++++ ...Type.java => RemoteStoreBlobPathType.java} | 8 ++--- .../MetadataCreateIndexServiceTests.java | 10 +++--- 4 files changed, 52 insertions(+), 18 deletions(-) create mode 100644 server/src/main/java/org/opensearch/index/remote/RemoteStoreBlobPathResolver.java rename server/src/main/java/org/opensearch/index/remote/{RemoteStorePathType.java => RemoteStoreBlobPathType.java} (78%) diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java index 34405c0647676..92d479ae8e66a 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java @@ -88,7 +88,8 @@ import org.opensearch.index.mapper.MapperService; import org.opensearch.index.mapper.MapperService.MergeReason; import org.opensearch.index.query.QueryShardContext; -import org.opensearch.index.remote.RemoteStorePathType; +import org.opensearch.index.remote.RemoteStoreBlobPathResolver; +import org.opensearch.index.remote.RemoteStoreBlobPathType; import org.opensearch.index.shard.IndexSettingProvider; import org.opensearch.index.translog.Translog; import org.opensearch.indices.IndexCreationException; @@ -168,6 +169,9 @@ public class MetadataCreateIndexService { private final ClusterManagerTaskThrottler.ThrottlingKey createIndexTaskKey; private AwarenessReplicaBalance awarenessReplicaBalance; + @Nullable + private final RemoteStoreBlobPathResolver remoteStoreBlobPathResolver; + public MetadataCreateIndexService( final Settings settings, final ClusterService clusterService, @@ -199,6 +203,9 @@ public MetadataCreateIndexService( // Task is onboarded for throttling, it will get retried from associated TransportClusterManagerNodeAction. createIndexTaskKey = clusterService.registerClusterManagerTask(ClusterManagerTaskKeys.CREATE_INDEX_KEY, true); + remoteStoreBlobPathResolver = isRemoteStoreAttributePresent(settings) + ? new RemoteStoreBlobPathResolver(clusterService.getClusterSettings()) + : null; } /** @@ -547,14 +554,9 @@ IndexMetadata buildAndValidateTemporaryIndexMetadata( tmpImdBuilder.settings(indexSettings); tmpImdBuilder.system(isSystem); - if (isRemoteStoreAttributePresent(settings)) { - String pathType; - if (clusterService.getClusterSettings().get(IndicesService.CLUSTER_REMOTE_STORE_PATH_PREFIX_OPTIMISED_SETTING)) { - pathType = RemoteStorePathType.HASHED_PREFIX.toString(); - } else { - pathType = RemoteStorePathType.FIXED.toString(); - } - tmpImdBuilder.putCustom(IndexMetadata.REMOTE_STORE_CUSTOM_KEY, Map.of(RemoteStorePathType.NAME, pathType)); + if (remoteStoreBlobPathResolver != null) { + String pathType = remoteStoreBlobPathResolver.resolveType().toString(); + tmpImdBuilder.putCustom(IndexMetadata.REMOTE_STORE_CUSTOM_KEY, Map.of(RemoteStoreBlobPathType.NAME, pathType)); } // Set up everything, now locally create the index to see that things are ok, and apply diff --git a/server/src/main/java/org/opensearch/index/remote/RemoteStoreBlobPathResolver.java b/server/src/main/java/org/opensearch/index/remote/RemoteStoreBlobPathResolver.java new file mode 100644 index 0000000000000..e1f08a9b1ecd1 --- /dev/null +++ b/server/src/main/java/org/opensearch/index/remote/RemoteStoreBlobPathResolver.java @@ -0,0 +1,32 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.index.remote; + +import org.opensearch.common.settings.ClusterSettings; +import org.opensearch.indices.IndicesService; + +/** + * Determines the {@link RemoteStoreBlobPathType} at the time of index metadata creation. + * + * @opensearch.internal + */ +public class RemoteStoreBlobPathResolver { + + private final ClusterSettings clusterSettings; + + public RemoteStoreBlobPathResolver(ClusterSettings clusterSettings) { + this.clusterSettings = clusterSettings; + } + + public RemoteStoreBlobPathType resolveType() { + return clusterSettings.get(IndicesService.CLUSTER_REMOTE_STORE_PATH_PREFIX_OPTIMISED_SETTING) + ? RemoteStoreBlobPathType.HASHED_PREFIX + : RemoteStoreBlobPathType.FIXED; + } +} diff --git a/server/src/main/java/org/opensearch/index/remote/RemoteStorePathType.java b/server/src/main/java/org/opensearch/index/remote/RemoteStoreBlobPathType.java similarity index 78% rename from server/src/main/java/org/opensearch/index/remote/RemoteStorePathType.java rename to server/src/main/java/org/opensearch/index/remote/RemoteStoreBlobPathType.java index 5547d84c1251c..fc3dac84b54c4 100644 --- a/server/src/main/java/org/opensearch/index/remote/RemoteStorePathType.java +++ b/server/src/main/java/org/opensearch/index/remote/RemoteStoreBlobPathType.java @@ -14,16 +14,16 @@ * * @opensearch.internal */ -public enum RemoteStorePathType { +public enum RemoteStoreBlobPathType { FIXED, HASHED_PREFIX; - public static RemoteStorePathType parseString(String remoteStorePathType) { + public static RemoteStoreBlobPathType parseString(String remoteStoreBlobPathType) { try { - return RemoteStorePathType.valueOf(remoteStorePathType); + return RemoteStoreBlobPathType.valueOf(remoteStoreBlobPathType); } catch (IllegalArgumentException e) { - throw new IllegalArgumentException("Could not parse RemoteStorePathType for [" + remoteStorePathType + "]"); + throw new IllegalArgumentException("Could not parse RemoteStorePathType for [" + remoteStoreBlobPathType + "]"); } catch (NullPointerException npe) { // return a default value for null input return FIXED; 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 c5924f1de5bdd..34ae53eb7a093 100644 --- a/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java +++ b/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java @@ -71,7 +71,7 @@ import org.opensearch.index.IndexSettings; import org.opensearch.index.mapper.MapperService; import org.opensearch.index.query.QueryShardContext; -import org.opensearch.index.remote.RemoteStorePathType; +import org.opensearch.index.remote.RemoteStoreBlobPathType; import org.opensearch.index.translog.Translog; import org.opensearch.indices.IndexCreationException; import org.opensearch.indices.IndicesService; @@ -1594,16 +1594,16 @@ public void testRemoteCustomData() { indexMetadata = testRemoteCustomData(true, false); validateRemoteCustomData( indexMetadata.getCustomData(IndexMetadata.REMOTE_STORE_CUSTOM_KEY), - RemoteStorePathType.NAME, - RemoteStorePathType.FIXED.toString() + RemoteStoreBlobPathType.NAME, + RemoteStoreBlobPathType.FIXED.toString() ); // Case 3 - cluster.remote_store.index.path.prefix.optimised=true indexMetadata = testRemoteCustomData(true, true); validateRemoteCustomData( indexMetadata.getCustomData(IndexMetadata.REMOTE_STORE_CUSTOM_KEY), - RemoteStorePathType.NAME, - RemoteStorePathType.HASHED_PREFIX.toString() + RemoteStoreBlobPathType.NAME, + RemoteStoreBlobPathType.HASHED_PREFIX.toString() ); } From 25e06e92c27db5dc3ffcc566383b399152b92c19 Mon Sep 17 00:00:00 2001 From: Ashish Singh Date: Mon, 18 Mar 2024 18:35:44 +0530 Subject: [PATCH 3/3] Incorporate PR review comments Signed-off-by: Ashish Singh --- .../metadata/MetadataCreateIndexService.java | 16 ++++++------ .../common/settings/ClusterSettings.java | 2 +- ...lver.java => RemoteStorePathResolver.java} | 12 ++++----- ...PathType.java => RemoteStorePathType.java} | 11 ++++---- .../opensearch/indices/IndicesService.java | 12 +++++---- .../MetadataCreateIndexServiceTests.java | 26 +++++++++---------- 6 files changed, 38 insertions(+), 41 deletions(-) rename server/src/main/java/org/opensearch/index/remote/{RemoteStoreBlobPathResolver.java => RemoteStorePathResolver.java} (58%) rename server/src/main/java/org/opensearch/index/remote/{RemoteStoreBlobPathType.java => RemoteStorePathType.java} (73%) diff --git a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java index a9140a7fec38e..f117d1a4a11a2 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java @@ -88,8 +88,8 @@ import org.opensearch.index.mapper.MapperService; import org.opensearch.index.mapper.MapperService.MergeReason; import org.opensearch.index.query.QueryShardContext; -import org.opensearch.index.remote.RemoteStoreBlobPathResolver; -import org.opensearch.index.remote.RemoteStoreBlobPathType; +import org.opensearch.index.remote.RemoteStorePathResolver; +import org.opensearch.index.remote.RemoteStorePathType; import org.opensearch.index.shard.IndexSettingProvider; import org.opensearch.index.translog.Translog; import org.opensearch.indices.IndexCreationException; @@ -170,7 +170,7 @@ public class MetadataCreateIndexService { private AwarenessReplicaBalance awarenessReplicaBalance; @Nullable - private final RemoteStoreBlobPathResolver remoteStoreBlobPathResolver; + private final RemoteStorePathResolver remoteStorePathResolver; public MetadataCreateIndexService( final Settings settings, @@ -203,8 +203,8 @@ public MetadataCreateIndexService( // Task is onboarded for throttling, it will get retried from associated TransportClusterManagerNodeAction. createIndexTaskKey = clusterService.registerClusterManagerTask(ClusterManagerTaskKeys.CREATE_INDEX_KEY, true); - remoteStoreBlobPathResolver = isRemoteDataAttributePresent(settings) - ? new RemoteStoreBlobPathResolver(clusterService.getClusterSettings()) + remoteStorePathResolver = isRemoteDataAttributePresent(settings) + ? new RemoteStorePathResolver(clusterService.getClusterSettings()) : null; } @@ -554,9 +554,9 @@ IndexMetadata buildAndValidateTemporaryIndexMetadata( tmpImdBuilder.settings(indexSettings); tmpImdBuilder.system(isSystem); - if (remoteStoreBlobPathResolver != null) { - String pathType = remoteStoreBlobPathResolver.resolveType().toString(); - tmpImdBuilder.putCustom(IndexMetadata.REMOTE_STORE_CUSTOM_KEY, Map.of(RemoteStoreBlobPathType.NAME, pathType)); + if (remoteStorePathResolver != null) { + String pathType = remoteStorePathResolver.resolveType().toString(); + tmpImdBuilder.putCustom(IndexMetadata.REMOTE_STORE_CUSTOM_KEY, Map.of(RemoteStorePathType.NAME, pathType)); } // Set up everything, now locally create the index to see that things are ok, and apply diff --git a/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java b/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java index d45152bd03ea5..730a330b6ca60 100644 --- a/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/ClusterSettings.java @@ -713,7 +713,7 @@ public void apply(Settings value, Settings current, Settings previous) { IoBasedAdmissionControllerSettings.SEARCH_IO_USAGE_LIMIT, IoBasedAdmissionControllerSettings.INDEXING_IO_USAGE_LIMIT, IndicesService.CLUSTER_INDEX_RESTRICT_REPLICATION_TYPE_SETTING, - IndicesService.CLUSTER_REMOTE_STORE_PATH_PREFIX_OPTIMISED_SETTING, + IndicesService.CLUSTER_REMOTE_STORE_PATH_PREFIX_TYPE_SETTING, // Concurrent segment search settings SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING, diff --git a/server/src/main/java/org/opensearch/index/remote/RemoteStoreBlobPathResolver.java b/server/src/main/java/org/opensearch/index/remote/RemoteStorePathResolver.java similarity index 58% rename from server/src/main/java/org/opensearch/index/remote/RemoteStoreBlobPathResolver.java rename to server/src/main/java/org/opensearch/index/remote/RemoteStorePathResolver.java index e1f08a9b1ecd1..6e8126fcce0ca 100644 --- a/server/src/main/java/org/opensearch/index/remote/RemoteStoreBlobPathResolver.java +++ b/server/src/main/java/org/opensearch/index/remote/RemoteStorePathResolver.java @@ -12,21 +12,19 @@ import org.opensearch.indices.IndicesService; /** - * Determines the {@link RemoteStoreBlobPathType} at the time of index metadata creation. + * Determines the {@link RemoteStorePathType} at the time of index metadata creation. * * @opensearch.internal */ -public class RemoteStoreBlobPathResolver { +public class RemoteStorePathResolver { private final ClusterSettings clusterSettings; - public RemoteStoreBlobPathResolver(ClusterSettings clusterSettings) { + public RemoteStorePathResolver(ClusterSettings clusterSettings) { this.clusterSettings = clusterSettings; } - public RemoteStoreBlobPathType resolveType() { - return clusterSettings.get(IndicesService.CLUSTER_REMOTE_STORE_PATH_PREFIX_OPTIMISED_SETTING) - ? RemoteStoreBlobPathType.HASHED_PREFIX - : RemoteStoreBlobPathType.FIXED; + public RemoteStorePathType resolveType() { + return clusterSettings.get(IndicesService.CLUSTER_REMOTE_STORE_PATH_PREFIX_TYPE_SETTING); } } diff --git a/server/src/main/java/org/opensearch/index/remote/RemoteStoreBlobPathType.java b/server/src/main/java/org/opensearch/index/remote/RemoteStorePathType.java similarity index 73% rename from server/src/main/java/org/opensearch/index/remote/RemoteStoreBlobPathType.java rename to server/src/main/java/org/opensearch/index/remote/RemoteStorePathType.java index fc3dac84b54c4..a64e07ab1f66f 100644 --- a/server/src/main/java/org/opensearch/index/remote/RemoteStoreBlobPathType.java +++ b/server/src/main/java/org/opensearch/index/remote/RemoteStorePathType.java @@ -8,25 +8,24 @@ package org.opensearch.index.remote; +import java.util.Locale; + /** * Enumerates the types of remote store paths resolution techniques supported by OpenSearch. * For more information, see Github issue #12567. * * @opensearch.internal */ -public enum RemoteStoreBlobPathType { +public enum RemoteStorePathType { FIXED, HASHED_PREFIX; - public static RemoteStoreBlobPathType parseString(String remoteStoreBlobPathType) { + public static RemoteStorePathType parseString(String remoteStoreBlobPathType) { try { - return RemoteStoreBlobPathType.valueOf(remoteStoreBlobPathType); + return RemoteStorePathType.valueOf(remoteStoreBlobPathType.toUpperCase(Locale.ROOT)); } catch (IllegalArgumentException e) { throw new IllegalArgumentException("Could not parse RemoteStorePathType for [" + remoteStoreBlobPathType + "]"); - } catch (NullPointerException npe) { - // return a default value for null input - return FIXED; } } diff --git a/server/src/main/java/org/opensearch/indices/IndicesService.java b/server/src/main/java/org/opensearch/indices/IndicesService.java index 07d1682ad6fb0..0e64894e6f708 100644 --- a/server/src/main/java/org/opensearch/indices/IndicesService.java +++ b/server/src/main/java/org/opensearch/indices/IndicesService.java @@ -123,6 +123,7 @@ import org.opensearch.index.query.QueryRewriteContext; import org.opensearch.index.recovery.RecoveryStats; import org.opensearch.index.refresh.RefreshStats; +import org.opensearch.index.remote.RemoteStorePathType; import org.opensearch.index.remote.RemoteStoreStatsTrackerFactory; import org.opensearch.index.search.stats.SearchStats; import org.opensearch.index.seqno.RetentionLeaseStats; @@ -314,12 +315,13 @@ public class IndicesService extends AbstractLifecycleComponent ); /** - * This setting is used to enable the optimisation in prefix path which helps in achieving higher throughput and lesser - * rate limiting by remote store providers. This setting is effective only for remote store enabled cluster. + * This setting is used to set the remote store blob store path prefix strategy. This setting is effective only for + * remote store enabled cluster. */ - public static final Setting CLUSTER_REMOTE_STORE_PATH_PREFIX_OPTIMISED_SETTING = Setting.boolSetting( - "cluster.remote_store.index.path.prefix.optimised", - false, + public static final Setting CLUSTER_REMOTE_STORE_PATH_PREFIX_TYPE_SETTING = new Setting<>( + "cluster.remote_store.index.path.prefix.type", + RemoteStorePathType.FIXED.toString(), + RemoteStorePathType::parseString, Property.NodeScope, Property.Dynamic ); 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 55b3cd72076dc..cf4de32890a2a 100644 --- a/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java +++ b/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java @@ -71,7 +71,7 @@ import org.opensearch.index.IndexSettings; import org.opensearch.index.mapper.MapperService; import org.opensearch.index.query.QueryShardContext; -import org.opensearch.index.remote.RemoteStoreBlobPathType; +import org.opensearch.index.remote.RemoteStorePathType; import org.opensearch.index.translog.Translog; import org.opensearch.indices.IndexCreationException; import org.opensearch.indices.IndicesService; @@ -1587,34 +1587,32 @@ public void testBuildIndexMetadata() { */ public void testRemoteCustomData() { // Case 1 - Remote store is not enabled - IndexMetadata indexMetadata = testRemoteCustomData(false, randomBoolean()); + IndexMetadata indexMetadata = testRemoteCustomData(false, randomFrom(RemoteStorePathType.values())); assertNull(indexMetadata.getCustomData(IndexMetadata.REMOTE_STORE_CUSTOM_KEY)); - // Case 2 - cluster.remote_store.index.path.prefix.optimised=false (default value) - indexMetadata = testRemoteCustomData(true, false); + // Case 2 - cluster.remote_store.index.path.prefix.optimised=fixed (default value) + indexMetadata = testRemoteCustomData(true, RemoteStorePathType.FIXED); validateRemoteCustomData( indexMetadata.getCustomData(IndexMetadata.REMOTE_STORE_CUSTOM_KEY), - RemoteStoreBlobPathType.NAME, - RemoteStoreBlobPathType.FIXED.toString() + RemoteStorePathType.NAME, + RemoteStorePathType.FIXED.toString() ); - // Case 3 - cluster.remote_store.index.path.prefix.optimised=true - indexMetadata = testRemoteCustomData(true, true); + // Case 3 - cluster.remote_store.index.path.prefix.optimised=hashed_prefix + indexMetadata = testRemoteCustomData(true, RemoteStorePathType.HASHED_PREFIX); validateRemoteCustomData( indexMetadata.getCustomData(IndexMetadata.REMOTE_STORE_CUSTOM_KEY), - RemoteStoreBlobPathType.NAME, - RemoteStoreBlobPathType.HASHED_PREFIX.toString() + RemoteStorePathType.NAME, + RemoteStorePathType.HASHED_PREFIX.toString() ); } - private IndexMetadata testRemoteCustomData(boolean remoteStoreEnabled, boolean optimisedPrefix) { + private IndexMetadata testRemoteCustomData(boolean remoteStoreEnabled, RemoteStorePathType remoteStorePathType) { Settings.Builder settingsBuilder = Settings.builder(); if (remoteStoreEnabled) { settingsBuilder.put(NODE_ATTRIBUTES.getKey() + REMOTE_STORE_SEGMENT_REPOSITORY_NAME_ATTRIBUTE_KEY, "test"); } - if (optimisedPrefix) { - settingsBuilder.put(IndicesService.CLUSTER_REMOTE_STORE_PATH_PREFIX_OPTIMISED_SETTING.getKey(), true); - } + settingsBuilder.put(IndicesService.CLUSTER_REMOTE_STORE_PATH_PREFIX_TYPE_SETTING.getKey(), remoteStorePathType.toString()); Settings settings = settingsBuilder.build(); ClusterService clusterService = mock(ClusterService.class);