From c588580aaa4bd238bb40f9e5a199178c7194aa9f Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 26 Apr 2024 12:22:59 +0000 Subject: [PATCH] Update Shallow Snapshot flows to support remote path type & hash algo (#12988) Signed-off-by: Ashish Singh (cherry picked from commit e713175d458ef2b5198945b6f32e0548572b7b87) Signed-off-by: github-actions[bot] --- .../remotestore/RemoteRestoreSnapshotIT.java | 46 +++- .../metadata/MetadataCreateIndexService.java | 40 ++-- .../org/opensearch/index/IndexSettings.java | 19 +- .../index/remote/RemoteStoreEnums.java | 81 ++++++- .../index/remote/RemoteStorePathStrategy.java | 17 +- .../RemoteStorePathStrategyResolver.java | 16 +- .../opensearch/index/shard/StoreRecovery.java | 5 +- .../RemoteStoreShardShallowCopySnapshot.java | 199 +++++++++++++----- .../blobstore/BlobStoreRepository.java | 15 +- .../MetadataCreateIndexServiceTests.java | 9 +- ...oteStoreShardShallowCopySnapshotTests.java | 186 ++++++++++++++-- .../RemoteSegmentStoreDirectoryTests.java | 6 +- 12 files changed, 508 insertions(+), 131 deletions(-) diff --git a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java index f7595d2bc9eb1..282fc70233e85 100644 --- a/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java +++ b/server/src/internalClusterTest/java/org/opensearch/remotestore/RemoteRestoreSnapshotIT.java @@ -18,6 +18,7 @@ import org.opensearch.client.Requests; import org.opensearch.cluster.ClusterState; import org.opensearch.cluster.metadata.IndexMetadata; +import org.opensearch.common.Nullable; import org.opensearch.common.io.PathUtils; import org.opensearch.common.settings.Settings; import org.opensearch.common.util.io.IOUtils; @@ -46,6 +47,7 @@ import java.util.Arrays; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.concurrent.ExecutionException; import java.util.stream.Collectors; @@ -226,7 +228,7 @@ public void testRemoteStoreCustomDataOnIndexCreationAndRestore() { indexDocuments(client, indexName1, randomIntBetween(5, 10)); ensureGreen(indexName1); - validatePathType(indexName1, PathType.FIXED, PathHashAlgorithm.FNV_1A); + validatePathType(indexName1, PathType.FIXED); logger.info("--> snapshot"); SnapshotInfo snapshotInfo = createSnapshot(snapshotRepoName, snapshotName1, new ArrayList<>(Arrays.asList(indexName1))); @@ -243,7 +245,7 @@ public void testRemoteStoreCustomDataOnIndexCreationAndRestore() { .get(); assertEquals(RestStatus.ACCEPTED, restoreSnapshotResponse.status()); ensureGreen(restoredIndexName1version1); - validatePathType(restoredIndexName1version1, PathType.FIXED, PathHashAlgorithm.FNV_1A); + validatePathType(restoredIndexName1version1, PathType.FIXED); client(clusterManagerNode).admin() .cluster() @@ -269,16 +271,50 @@ public void testRemoteStoreCustomDataOnIndexCreationAndRestore() { validatePathType(indexName2, PathType.HASHED_PREFIX, PathHashAlgorithm.FNV_1A); // Validating that custom data has not changed for indexes which were created before the cluster setting got updated - validatePathType(indexName1, PathType.FIXED, PathHashAlgorithm.FNV_1A); + validatePathType(indexName1, PathType.FIXED); + + // Create Snapshot of index 2 + String snapshotName2 = "test-restore-snapshot2"; + snapshotInfo = createSnapshot(snapshotRepoName, snapshotName2, new ArrayList<>(List.of(indexName2))); + assertEquals(SnapshotState.SUCCESS, snapshotInfo.state()); + assertTrue(snapshotInfo.successfulShards() > 0); + assertEquals(snapshotInfo.totalShards(), snapshotInfo.successfulShards()); + + // Update cluster settings to FIXED + client(clusterManagerNode).admin() + .cluster() + .prepareUpdateSettings() + .setTransientSettings(Settings.builder().put(CLUSTER_REMOTE_STORE_PATH_PREFIX_TYPE_SETTING.getKey(), PathType.FIXED)) + .get(); + + // Close index 2 + assertAcked(client().admin().indices().prepareClose(indexName2)); + restoreSnapshotResponse = client.admin() + .cluster() + .prepareRestoreSnapshot(snapshotRepoName, snapshotName2) + .setWaitForCompletion(false) + .setIndices(indexName2) + .get(); + assertEquals(RestStatus.ACCEPTED, restoreSnapshotResponse.status()); + ensureGreen(indexName2); + + // Validating that custom data has not changed for testindex2 which was created before the cluster setting got updated + validatePathType(indexName2, PathType.HASHED_PREFIX, PathHashAlgorithm.FNV_1A); } - private void validatePathType(String index, PathType pathType, PathHashAlgorithm pathHashAlgorithm) { + private void validatePathType(String index, PathType pathType) { + validatePathType(index, pathType, null); + } + + private void validatePathType(String index, PathType pathType, @Nullable PathHashAlgorithm pathHashAlgorithm) { ClusterState state = client().admin().cluster().prepareState().execute().actionGet().getState(); // Validate that the remote_store custom data is present in index metadata for the created index. Map remoteCustomData = state.metadata().index(index).getCustomData(IndexMetadata.REMOTE_STORE_CUSTOM_KEY); assertNotNull(remoteCustomData); assertEquals(pathType.name(), remoteCustomData.get(PathType.NAME)); - assertEquals(pathHashAlgorithm.name(), remoteCustomData.get(PathHashAlgorithm.NAME)); + if (Objects.nonNull(pathHashAlgorithm)) { + assertEquals(pathHashAlgorithm.name(), remoteCustomData.get(PathHashAlgorithm.NAME)); + } } public void testRestoreInSameRemoteStoreEnabledIndex() throws IOException { 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 3e656ac089c2d..a7cda09c9ac23 100644 --- a/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java +++ b/server/src/main/java/org/opensearch/cluster/metadata/MetadataCreateIndexService.java @@ -210,8 +210,9 @@ public MetadataCreateIndexService( // Task is onboarded for throttling, it will get retried from associated TransportClusterManagerNodeAction. createIndexTaskKey = clusterService.registerClusterManagerTask(ClusterManagerTaskKeys.CREATE_INDEX_KEY, true); + Supplier minNodeVersionSupplier = () -> clusterService.state().nodes().getMinNodeVersion(); remoteStorePathStrategyResolver = isRemoteDataAttributePresent(settings) - ? new RemoteStorePathStrategyResolver(clusterService.getClusterSettings()) + ? new RemoteStorePathStrategyResolver(clusterService.getClusterSettings(), minNodeVersionSupplier) : null; } @@ -576,28 +577,23 @@ IndexMetadata buildAndValidateTemporaryIndexMetadata( * @param assertNullOldType flag to verify that the old remote store path type is null */ public void addRemoteStorePathStrategyInCustomData(IndexMetadata.Builder tmpImdBuilder, boolean assertNullOldType) { - if (remoteStorePathStrategyResolver != null) { - // It is possible that remote custom data exists already. In such cases, we need to only update the path type - // in the remote store custom data map. - Map existingRemoteCustomData = tmpImdBuilder.removeCustom(IndexMetadata.REMOTE_STORE_CUSTOM_KEY); - Map remoteCustomData = existingRemoteCustomData == null - ? new HashMap<>() - : new HashMap<>(existingRemoteCustomData); - // Determine the path type for use using the remoteStorePathResolver. - RemoteStorePathStrategy newPathStrategy = remoteStorePathStrategyResolver.get(); - String oldPathType = remoteCustomData.put(PathType.NAME, newPathStrategy.getType().name()); - String oldHashAlgorithm = remoteCustomData.put(PathHashAlgorithm.NAME, newPathStrategy.getHashAlgorithm().name()); - assert !assertNullOldType || (Objects.isNull(oldPathType) && Objects.isNull(oldHashAlgorithm)); - logger.trace( - () -> new ParameterizedMessage( - "Added newPathStrategy={}, replaced oldPathType={} oldHashAlgorithm={}", - newPathStrategy, - oldPathType, - oldHashAlgorithm - ) - ); - tmpImdBuilder.putCustom(IndexMetadata.REMOTE_STORE_CUSTOM_KEY, remoteCustomData); + if (remoteStorePathStrategyResolver == null) { + return; + } + // It is possible that remote custom data exists already. In such cases, we need to only update the path type + // in the remote store custom data map. + Map existingCustomData = tmpImdBuilder.removeCustom(IndexMetadata.REMOTE_STORE_CUSTOM_KEY); + assert assertNullOldType == false || Objects.isNull(existingCustomData); + + // Determine the path type for use using the remoteStorePathResolver. + RemoteStorePathStrategy newPathStrategy = remoteStorePathStrategyResolver.get(); + Map remoteCustomData = new HashMap<>(); + remoteCustomData.put(PathType.NAME, newPathStrategy.getType().name()); + if (Objects.nonNull(newPathStrategy.getHashAlgorithm())) { + remoteCustomData.put(PathHashAlgorithm.NAME, newPathStrategy.getHashAlgorithm().name()); } + logger.trace(() -> new ParameterizedMessage("Added newStrategy={}, replaced oldStrategy={}", remoteCustomData, existingCustomData)); + tmpImdBuilder.putCustom(IndexMetadata.REMOTE_STORE_CUSTOM_KEY, remoteCustomData); } private ClusterState applyCreateIndexRequestWithV1Templates( diff --git a/server/src/main/java/org/opensearch/index/IndexSettings.java b/server/src/main/java/org/opensearch/index/IndexSettings.java index b5cccb61c7a5e..6419a1d826100 100644 --- a/server/src/main/java/org/opensearch/index/IndexSettings.java +++ b/server/src/main/java/org/opensearch/index/IndexSettings.java @@ -64,6 +64,7 @@ import java.util.List; import java.util.Locale; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.concurrent.TimeUnit; import java.util.function.Consumer; @@ -764,6 +765,7 @@ public static IndexMergePolicy fromString(String text) { private volatile String defaultSearchPipeline; private final boolean widenIndexSortType; private final boolean assignedOnRemoteNode; + private final RemoteStorePathStrategy remoteStorePathStrategy; /** * The maximum age of a retention lease before it is considered expired. @@ -987,6 +989,7 @@ public IndexSettings(final IndexMetadata indexMetadata, final Settings nodeSetti */ widenIndexSortType = IndexMetadata.SETTING_INDEX_VERSION_CREATED.get(settings).before(V_2_7_0); assignedOnRemoteNode = RemoteStoreNodeAttribute.isRemoteDataAttributePresent(this.getNodeSettings()); + remoteStorePathStrategy = determineRemoteStorePathStrategy(); setEnableFuzzySetForDocId(scopedSettings.get(INDEX_DOC_ID_FUZZY_SET_ENABLED_SETTING)); setDocIdFuzzySetFalsePositiveProbability(scopedSettings.get(INDEX_DOC_ID_FUZZY_SET_FALSE_POSITIVE_PROBABILITY_SETTING)); @@ -1908,15 +1911,19 @@ public void setDocIdFuzzySetFalsePositiveProbability(double docIdFuzzySetFalsePo this.docIdFuzzySetFalsePositiveProbability = docIdFuzzySetFalsePositiveProbability; } - public RemoteStorePathStrategy getRemoteStorePathStrategy() { + private RemoteStorePathStrategy determineRemoteStorePathStrategy() { Map remoteCustomData = indexMetadata.getCustomData(IndexMetadata.REMOTE_STORE_CUSTOM_KEY); - if (remoteCustomData != null - && remoteCustomData.containsKey(PathType.NAME) - && remoteCustomData.containsKey(PathHashAlgorithm.NAME)) { + assert remoteCustomData == null || remoteCustomData.containsKey(PathType.NAME); + if (remoteCustomData != null && remoteCustomData.containsKey(PathType.NAME)) { PathType pathType = PathType.parseString(remoteCustomData.get(PathType.NAME)); - PathHashAlgorithm pathHashAlgorithm = PathHashAlgorithm.parseString(remoteCustomData.get(PathHashAlgorithm.NAME)); - return new RemoteStorePathStrategy(pathType, pathHashAlgorithm); + String hashAlgoStr = remoteCustomData.get(PathHashAlgorithm.NAME); + PathHashAlgorithm hashAlgorithm = Objects.nonNull(hashAlgoStr) ? PathHashAlgorithm.parseString(hashAlgoStr) : null; + return new RemoteStorePathStrategy(pathType, hashAlgorithm); } return new RemoteStorePathStrategy(PathType.FIXED); } + + public RemoteStorePathStrategy getRemoteStorePathStrategy() { + return remoteStorePathStrategy; + } } diff --git a/server/src/main/java/org/opensearch/index/remote/RemoteStoreEnums.java b/server/src/main/java/org/opensearch/index/remote/RemoteStoreEnums.java index 4e557d8c24431..30cfc054e3d0a 100644 --- a/server/src/main/java/org/opensearch/index/remote/RemoteStoreEnums.java +++ b/server/src/main/java/org/opensearch/index/remote/RemoteStoreEnums.java @@ -8,14 +8,19 @@ package org.opensearch.index.remote; +import org.apache.logging.log4j.message.ParameterizedMessage; import org.opensearch.common.annotation.PublicApi; import org.opensearch.common.blobstore.BlobPath; import org.opensearch.common.hash.FNV1a; import org.opensearch.index.remote.RemoteStorePathStrategy.PathInput; +import java.util.HashMap; import java.util.Locale; +import java.util.Map; +import java.util.Objects; import java.util.Set; +import static java.util.Collections.unmodifiableMap; import static org.opensearch.index.remote.RemoteStoreEnums.DataType.DATA; import static org.opensearch.index.remote.RemoteStoreEnums.DataType.METADATA; @@ -78,9 +83,10 @@ public String getName() { */ @PublicApi(since = "2.14.0") public enum PathType { - FIXED { + FIXED(0) { @Override public BlobPath generatePath(PathInput pathInput, PathHashAlgorithm hashAlgorithm) { + assert Objects.isNull(hashAlgorithm) : "hashAlgorithm is expected to be null with fixed remote store path type"; // Hash algorithm is not used in FIXED path type return pathInput.basePath() .add(pathInput.indexUUID()) @@ -94,7 +100,7 @@ boolean requiresHashAlgorithm() { return false; } }, - HASHED_PREFIX { + HASHED_PREFIX(1) { @Override public BlobPath generatePath(PathInput pathInput, PathHashAlgorithm hashAlgorithm) { // TODO - We need to implement this, keeping the same path as Fixed for sake of multiple tests that can fail otherwise. @@ -112,6 +118,40 @@ boolean requiresHashAlgorithm() { } }; + private final int code; + + PathType(int code) { + this.code = code; + } + + public int getCode() { + return code; + } + + private static final Map CODE_TO_ENUM; + + static { + PathType[] values = values(); + Map codeToStatus = new HashMap<>(values.length); + for (PathType value : values) { + int code = value.code; + if (codeToStatus.containsKey(code)) { + throw new IllegalStateException( + new ParameterizedMessage("{} has same code as {}", codeToStatus.get(code), value).getFormattedMessage() + ); + } + codeToStatus.put(code, value); + } + CODE_TO_ENUM = unmodifiableMap(codeToStatus); + } + + /** + * Turn a status code into a {@link PathType}. + */ + public static PathType fromCode(int code) { + return CODE_TO_ENUM.get(code); + } + /** * This method generates the path for the given path input which constitutes multiple fields and characteristics * of the data. @@ -131,7 +171,7 @@ public BlobPath path(PathInput pathInput, PathHashAlgorithm hashAlgorithm) { return generatePath(pathInput, hashAlgorithm); } - abstract BlobPath generatePath(PathInput pathInput, PathHashAlgorithm hashAlgorithm); + protected abstract BlobPath generatePath(PathInput pathInput, PathHashAlgorithm hashAlgorithm); abstract boolean requiresHashAlgorithm(); @@ -158,7 +198,7 @@ public static PathType parseString(String pathType) { @PublicApi(since = "2.14.0") public enum PathHashAlgorithm { - FNV_1A { + FNV_1A(0) { @Override long hash(PathInput pathInput) { String input = pathInput.indexUUID() + pathInput.shardId() + pathInput.dataCategory().getName() + pathInput.dataType() @@ -167,6 +207,39 @@ long hash(PathInput pathInput) { } }; + private final int code; + + PathHashAlgorithm(int code) { + this.code = code; + } + + public int getCode() { + return code; + } + + private static final Map CODE_TO_ENUM; + static { + PathHashAlgorithm[] values = values(); + Map codeToStatus = new HashMap<>(values.length); + for (PathHashAlgorithm value : values) { + int code = value.code; + if (codeToStatus.containsKey(code)) { + throw new IllegalStateException( + new ParameterizedMessage("{} has same code as {}", codeToStatus.get(code), value).getFormattedMessage() + ); + } + codeToStatus.put(code, value); + } + CODE_TO_ENUM = unmodifiableMap(codeToStatus); + } + + /** + * Turn a status code into a {@link PathHashAlgorithm}. + */ + public static PathHashAlgorithm fromCode(int code) { + return CODE_TO_ENUM.get(code); + } + abstract long hash(PathInput pathInput); public static PathHashAlgorithm parseString(String pathHashAlgorithm) { diff --git a/server/src/main/java/org/opensearch/index/remote/RemoteStorePathStrategy.java b/server/src/main/java/org/opensearch/index/remote/RemoteStorePathStrategy.java index ce5a6748fd9d4..775f8fe19e4ef 100644 --- a/server/src/main/java/org/opensearch/index/remote/RemoteStorePathStrategy.java +++ b/server/src/main/java/org/opensearch/index/remote/RemoteStorePathStrategy.java @@ -8,6 +8,7 @@ package org.opensearch.index.remote; +import org.apache.logging.log4j.message.ParameterizedMessage; import org.opensearch.common.Nullable; import org.opensearch.common.annotation.PublicApi; import org.opensearch.common.blobstore.BlobPath; @@ -36,11 +37,21 @@ public RemoteStorePathStrategy(PathType type) { } public RemoteStorePathStrategy(PathType type, PathHashAlgorithm hashAlgorithm) { - assert type.requiresHashAlgorithm() == false || Objects.nonNull(hashAlgorithm); - this.type = Objects.requireNonNull(type); + Objects.requireNonNull(type, "pathType can not be null"); + if (isCompatible(type, hashAlgorithm) == false) { + throw new IllegalArgumentException( + new ParameterizedMessage("pathType={} pathHashAlgorithm={} are incompatible", type, hashAlgorithm).getFormattedMessage() + ); + } + this.type = type; this.hashAlgorithm = hashAlgorithm; } + public static boolean isCompatible(PathType type, PathHashAlgorithm hashAlgorithm) { + return (type.requiresHashAlgorithm() == false && Objects.isNull(hashAlgorithm)) + || (type.requiresHashAlgorithm() && Objects.nonNull(hashAlgorithm)); + } + public PathType getType() { return type; } @@ -55,7 +66,7 @@ public String toString() { } public BlobPath generatePath(PathInput pathInput) { - return type.generatePath(pathInput, hashAlgorithm); + return type.path(pathInput, hashAlgorithm); } /** diff --git a/server/src/main/java/org/opensearch/index/remote/RemoteStorePathStrategyResolver.java b/server/src/main/java/org/opensearch/index/remote/RemoteStorePathStrategyResolver.java index 20fc516132220..5b067115df781 100644 --- a/server/src/main/java/org/opensearch/index/remote/RemoteStorePathStrategyResolver.java +++ b/server/src/main/java/org/opensearch/index/remote/RemoteStorePathStrategyResolver.java @@ -8,11 +8,14 @@ package org.opensearch.index.remote; +import org.opensearch.Version; import org.opensearch.common.settings.ClusterSettings; import org.opensearch.index.remote.RemoteStoreEnums.PathHashAlgorithm; import org.opensearch.index.remote.RemoteStoreEnums.PathType; import org.opensearch.indices.IndicesService; +import java.util.function.Supplier; + /** * Determines the {@link RemoteStorePathStrategy} at the time of index metadata creation. * @@ -22,13 +25,22 @@ public class RemoteStorePathStrategyResolver { private volatile PathType type; - public RemoteStorePathStrategyResolver(ClusterSettings clusterSettings) { + private final Supplier minNodeVersionSupplier; + + public RemoteStorePathStrategyResolver(ClusterSettings clusterSettings, Supplier minNodeVersionSupplier) { + this.minNodeVersionSupplier = minNodeVersionSupplier; type = clusterSettings.get(IndicesService.CLUSTER_REMOTE_STORE_PATH_PREFIX_TYPE_SETTING); clusterSettings.addSettingsUpdateConsumer(IndicesService.CLUSTER_REMOTE_STORE_PATH_PREFIX_TYPE_SETTING, this::setType); } public RemoteStorePathStrategy get() { - return new RemoteStorePathStrategy(type, PathHashAlgorithm.FNV_1A); + PathType pathType; + PathHashAlgorithm pathHashAlgorithm; + // Min node version check ensures that we are enabling the new prefix type only when all the nodes understand it. + pathType = Version.CURRENT.compareTo(minNodeVersionSupplier.get()) <= 0 ? type : PathType.FIXED; + // If the path type is fixed, hash algorithm is not applicable. + pathHashAlgorithm = pathType == PathType.FIXED ? null : PathHashAlgorithm.FNV_1A; + return new RemoteStorePathStrategy(pathType, pathHashAlgorithm); } private void setType(PathType type) { diff --git a/server/src/main/java/org/opensearch/index/shard/StoreRecovery.java b/server/src/main/java/org/opensearch/index/shard/StoreRecovery.java index 879aa2b8d6c4f..e130f50e105d5 100644 --- a/server/src/main/java/org/opensearch/index/shard/StoreRecovery.java +++ b/server/src/main/java/org/opensearch/index/shard/StoreRecovery.java @@ -58,8 +58,6 @@ import org.opensearch.index.engine.Engine; import org.opensearch.index.engine.EngineException; import org.opensearch.index.mapper.MapperService; -import org.opensearch.index.remote.RemoteStoreEnums.PathType; -import org.opensearch.index.remote.RemoteStorePathStrategy; import org.opensearch.index.seqno.SequenceNumbers; import org.opensearch.index.snapshots.IndexShardRestoreFailedException; import org.opensearch.index.snapshots.blobstore.RemoteStoreShardShallowCopySnapshot; @@ -405,8 +403,7 @@ void recoverFromSnapshotAndRemoteStore( remoteStoreRepository, indexUUID, shardId, - new RemoteStorePathStrategy(PathType.FIXED) - // TODO - The path type needs to be obtained from RemoteStoreShardShallowCopySnapshot + shallowCopyShardMetadata.getRemoteStorePathStrategy() ); sourceRemoteDirectory.initializeToSpecificCommit( primaryTerm, diff --git a/server/src/main/java/org/opensearch/index/snapshots/blobstore/RemoteStoreShardShallowCopySnapshot.java b/server/src/main/java/org/opensearch/index/snapshots/blobstore/RemoteStoreShardShallowCopySnapshot.java index d54e9686ab951..9c0ea42810e16 100644 --- a/server/src/main/java/org/opensearch/index/snapshots/blobstore/RemoteStoreShardShallowCopySnapshot.java +++ b/server/src/main/java/org/opensearch/index/snapshots/blobstore/RemoteStoreShardShallowCopySnapshot.java @@ -8,17 +8,22 @@ package org.opensearch.index.snapshots.blobstore; +import org.apache.logging.log4j.message.ParameterizedMessage; import org.opensearch.OpenSearchParseException; import org.opensearch.common.annotation.PublicApi; import org.opensearch.core.ParseField; import org.opensearch.core.xcontent.ToXContentFragment; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.index.remote.RemoteStoreEnums.PathHashAlgorithm; +import org.opensearch.index.remote.RemoteStoreEnums.PathType; +import org.opensearch.index.remote.RemoteStorePathStrategy; import org.opensearch.index.snapshots.IndexShardSnapshotStatus; import java.io.IOException; import java.util.ArrayList; import java.util.List; +import java.util.Objects; /** * Remote Store based Shard snapshot metadata @@ -41,8 +46,10 @@ public class RemoteStoreShardShallowCopySnapshot implements ToXContentFragment, private final String repositoryBasePath; private final String indexUUID; private final List fileNames; + private final PathType pathType; + private final PathHashAlgorithm pathHashAlgorithm; - static final String DEFAULT_VERSION = "1"; + static final String DEFAULT_VERSION = "2"; static final String NAME = "name"; static final String VERSION = "version"; static final String INDEX_VERSION = "index_version"; @@ -61,6 +68,8 @@ public class RemoteStoreShardShallowCopySnapshot implements ToXContentFragment, static final String TOTAL_FILE_COUNT = "number_of_files"; static final String TOTAL_SIZE = "total_size"; + static final String PATH_TYPE = "path_type"; + static final String PATH_HASH_ALGORITHM = "path_hash_algorithm"; private static final ParseField PARSE_NAME = new ParseField(NAME); private static final ParseField PARSE_VERSION = new ParseField(VERSION); @@ -75,6 +84,8 @@ public class RemoteStoreShardShallowCopySnapshot implements ToXContentFragment, private static final ParseField PARSE_REMOTE_STORE_REPOSITORY = new ParseField(REMOTE_STORE_REPOSITORY); private static final ParseField PARSE_REPOSITORY_BASE_PATH = new ParseField(REPOSITORY_BASE_PATH); private static final ParseField PARSE_FILE_NAMES = new ParseField(FILE_NAMES); + private static final ParseField PARSE_PATH_TYPE = new ParseField(PATH_TYPE); + private static final ParseField PARSE_PATH_HASH_ALGORITHM = new ParseField(PATH_HASH_ALGORITHM); /** * Serializes shard snapshot metadata info into JSON @@ -101,6 +112,14 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.value(fileName); } builder.endArray(); + // We are handling NP check since a cluster can have indexes created earlier which do not have remote store + // path type and path hash algorithm in its custom data in index metadata. + if (Objects.nonNull(pathType)) { + builder.field(PATH_TYPE, pathType.getCode()); + } + if (Objects.nonNull(pathHashAlgorithm)) { + builder.field(PATH_HASH_ALGORITHM, pathHashAlgorithm.getCode()); + } return builder; } @@ -116,34 +135,30 @@ public RemoteStoreShardShallowCopySnapshot( String indexUUID, String remoteStoreRepository, String repositoryBasePath, - List fileNames + List fileNames, + PathType pathType, + PathHashAlgorithm pathHashAlgorithm ) { - this.version = DEFAULT_VERSION; - verifyParameters( - version, + this( + DEFAULT_VERSION, snapshot, indexVersion, primaryTerm, commitGeneration, + startTime, + time, + totalFileCount, + totalSize, indexUUID, remoteStoreRepository, - repositoryBasePath + repositoryBasePath, + fileNames, + pathType, + pathHashAlgorithm ); - this.snapshot = snapshot; - this.indexVersion = indexVersion; - this.primaryTerm = primaryTerm; - this.commitGeneration = commitGeneration; - this.startTime = startTime; - this.time = time; - this.totalFileCount = totalFileCount; - this.totalSize = totalSize; - this.indexUUID = indexUUID; - this.remoteStoreRepository = remoteStoreRepository; - this.repositoryBasePath = repositoryBasePath; - this.fileNames = fileNames; } - private RemoteStoreShardShallowCopySnapshot( + RemoteStoreShardShallowCopySnapshot( String version, String snapshot, long indexVersion, @@ -156,7 +171,9 @@ private RemoteStoreShardShallowCopySnapshot( String indexUUID, String remoteStoreRepository, String repositoryBasePath, - List fileNames + List fileNames, + PathType pathType, + PathHashAlgorithm pathHashAlgorithm ) { verifyParameters( version, @@ -166,7 +183,9 @@ private RemoteStoreShardShallowCopySnapshot( commitGeneration, indexUUID, remoteStoreRepository, - repositoryBasePath + repositoryBasePath, + pathType, + pathHashAlgorithm ); this.version = version; this.snapshot = snapshot; @@ -181,6 +200,8 @@ private RemoteStoreShardShallowCopySnapshot( this.remoteStoreRepository = remoteStoreRepository; this.repositoryBasePath = repositoryBasePath; this.fileNames = fileNames; + this.pathType = pathType; + this.pathHashAlgorithm = pathHashAlgorithm; } /** @@ -203,6 +224,8 @@ public static RemoteStoreShardShallowCopySnapshot fromXContent(XContentParser pa long primaryTerm = -1; long commitGeneration = -1; List fileNames = new ArrayList<>(); + PathType pathType = null; + PathHashAlgorithm pathHashAlgorithm = null; if (parser.currentToken() == null) { // fresh parser? move to the first token parser.nextToken(); @@ -237,6 +260,10 @@ public static RemoteStoreShardShallowCopySnapshot fromXContent(XContentParser pa remoteStoreRepository = parser.text(); } else if (PARSE_REPOSITORY_BASE_PATH.match(currentFieldName, parser.getDeprecationHandler())) { repositoryBasePath = parser.text(); + } else if (PARSE_PATH_TYPE.match(currentFieldName, parser.getDeprecationHandler())) { + pathType = PathType.fromCode(parser.intValue()); + } else if (PARSE_PATH_HASH_ALGORITHM.match(currentFieldName, parser.getDeprecationHandler())) { + pathHashAlgorithm = PathHashAlgorithm.fromCode(parser.intValue()); } else { throw new OpenSearchParseException("unknown parameter [{}]", currentFieldName); } @@ -266,7 +293,9 @@ public static RemoteStoreShardShallowCopySnapshot fromXContent(XContentParser pa indexUUID, remoteStoreRepository, repositoryBasePath, - fileNames + fileNames, + pathType, + pathHashAlgorithm ); } @@ -380,38 +409,47 @@ private void verifyParameters( long commitGeneration, String indexUUID, String remoteStoreRepository, - String repositoryBasePath + String repositoryBasePath, + PathType pathType, + PathHashAlgorithm pathHashAlgorithm ) { - String exceptionStr = null; - if (version == null) { - exceptionStr = "Invalid Version Provided"; - } - if (snapshot == null) { - exceptionStr = "Invalid/Missing Snapshot Name"; - } - if (indexVersion < 0) { - exceptionStr = "Invalid Index Version"; - } - if (primaryTerm < 0) { - exceptionStr = "Invalid Primary Term"; - } - if (commitGeneration < 0) { - exceptionStr = "Invalid Commit Generation"; - } - if (indexUUID == null) { - exceptionStr = "Invalid/Missing Index UUID"; - } - if (remoteStoreRepository == null) { - exceptionStr = "Invalid/Missing Remote Store Repository"; - } - if (repositoryBasePath == null) { - exceptionStr = "Invalid/Missing Repository Base Path"; - } - if (exceptionStr != null) { + + throwExceptionIfInvalid(Objects.isNull(version), "Invalid Version Provided"); + throwExceptionIfInvalid(Objects.isNull(snapshot), "Invalid/Missing Snapshot Name"); + throwExceptionIfInvalid(indexVersion < 0, "Invalid Index Version"); + throwExceptionIfInvalid(primaryTerm < 0, "Invalid Primary Term"); + throwExceptionIfInvalid(commitGeneration < 0, "Invalid Commit Generation"); + throwExceptionIfInvalid(Objects.isNull(indexUUID), "Invalid/Missing Index UUID"); + throwExceptionIfInvalid(Objects.isNull(remoteStoreRepository), "Invalid/Missing Remote Store Repository"); + throwExceptionIfInvalid(Objects.isNull(repositoryBasePath), "Invalid/Missing Repository Base Path"); + throwExceptionIfInvalid( + isValidRemotePathConfiguration(version, pathType, pathHashAlgorithm) == false, + new ParameterizedMessage( + "Invalid combination of pathType={} pathHashAlgorithm={} for version={}", + pathType, + pathHashAlgorithm, + version + ).getFormattedMessage() + ); + } + + private void throwExceptionIfInvalid(boolean isInvalid, String exceptionStr) { + if (isInvalid) { throw new IllegalArgumentException(exceptionStr); } } + private boolean isValidRemotePathConfiguration(String version, PathType pathType, PathHashAlgorithm pathHashAlgorithm) { + switch (version) { + case "1": + return Objects.isNull(pathType) && Objects.isNull(pathHashAlgorithm); + case "2": + return Objects.nonNull(pathType) && RemoteStorePathStrategy.isCompatible(pathType, pathHashAlgorithm); + default: + return false; + } + } + /** * Creates a new instance which has a different name and zero incremental file counts but is identical to this instance in terms of the files * it references. @@ -433,7 +471,9 @@ public RemoteStoreShardShallowCopySnapshot asClone(String targetSnapshotName, lo indexUUID, remoteStoreRepository, repositoryBasePath, - fileNames + fileNames, + pathType, + pathHashAlgorithm ); } @@ -449,4 +489,63 @@ public IndexShardSnapshotStatus getIndexShardSnapshotStatus() { null ); // Not adding a real generation here as it doesn't matter to callers } + + public PathType getPathType() { + return pathType; + } + + public PathHashAlgorithm getPathHashAlgorithm() { + return pathHashAlgorithm; + } + + public RemoteStorePathStrategy getRemoteStorePathStrategy() { + if (Objects.nonNull(pathType)) { + return new RemoteStorePathStrategy(pathType, pathHashAlgorithm); + } + return new RemoteStorePathStrategy(PathType.FIXED); + } + + @Override + public boolean equals(Object obj) { + if (this == obj) return true; + if (obj == null || getClass() != obj.getClass()) return false; + RemoteStoreShardShallowCopySnapshot that = (RemoteStoreShardShallowCopySnapshot) obj; + + return Objects.equals(this.snapshot, that.snapshot) + && Objects.equals(this.version, that.version) + && this.indexVersion == that.indexVersion + && this.startTime == that.startTime + && this.time == that.time + && this.totalFileCount == that.totalFileCount + && this.totalSize == that.totalSize + && this.primaryTerm == that.primaryTerm + && this.commitGeneration == that.commitGeneration + && Objects.equals(this.remoteStoreRepository, that.remoteStoreRepository) + && Objects.equals(this.repositoryBasePath, that.repositoryBasePath) + && Objects.equals(this.indexUUID, that.indexUUID) + && Objects.equals(this.fileNames, that.fileNames) + && Objects.equals(this.pathType, that.pathType) + && Objects.equals(this.pathHashAlgorithm, that.pathHashAlgorithm); + } + + @Override + public int hashCode() { + return Objects.hash( + snapshot, + version, + indexVersion, + startTime, + time, + totalFileCount, + totalSize, + primaryTerm, + commitGeneration, + remoteStoreRepository, + repositoryBasePath, + indexUUID, + fileNames, + pathType, + pathHashAlgorithm + ); + } } diff --git a/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java b/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java index 53514c8830e33..6298eb15550bf 100644 --- a/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java +++ b/server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java @@ -108,7 +108,6 @@ import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.core.xcontent.XContentParser; import org.opensearch.index.mapper.MapperService; -import org.opensearch.index.remote.RemoteStoreEnums.PathType; import org.opensearch.index.remote.RemoteStorePathStrategy; import org.opensearch.index.snapshots.IndexShardRestoreFailedException; import org.opensearch.index.snapshots.IndexShardSnapshotStatus; @@ -676,8 +675,7 @@ public void cloneRemoteStoreIndexShardSnapshot( remoteStoreRepository, indexUUID, String.valueOf(shardId.shardId()), - new RemoteStorePathStrategy(PathType.FIXED) - // TODO - The path type needs to be obtained from RemoteStoreShardShallowCopySnapshot + remStoreBasedShardMetadata.getRemoteStorePathStrategy() ); remoteStoreMetadataLockManger.cloneLock( FileLockInfo.getLockInfoBuilder().withAcquirerId(source.getUUID()).build(), @@ -1217,8 +1215,7 @@ protected void releaseRemoteStoreLockAndCleanup( remoteStoreRepoForIndex, indexUUID, shardId, - new RemoteStorePathStrategy(PathType.FIXED) - // TODO - The path type needs to be obtained from RemoteStoreShardShallowCopySnapshot + remoteStoreShardShallowCopySnapshot.getRemoteStorePathStrategy() ); remoteStoreMetadataLockManager.release(FileLockInfo.getLockInfoBuilder().withAcquirerId(shallowSnapshotUUID).build()); logger.debug("Successfully released lock for shard {} of index with uuid {}", shardId, indexUUID); @@ -1241,8 +1238,7 @@ protected void releaseRemoteStoreLockAndCleanup( indexUUID, new ShardId(Index.UNKNOWN_INDEX_NAME, indexUUID, Integer.parseInt(shardId)), ThreadPool.Names.REMOTE_PURGE, - new RemoteStorePathStrategy(PathType.FIXED) - // TODO - The path type needs to be obtained from RemoteStoreShardShallowCopySnapshot + remoteStoreShardShallowCopySnapshot.getRemoteStorePathStrategy() ); } } @@ -2781,6 +2777,7 @@ public void snapshotRemoteStoreIndexShard( // now create and write the commit point logger.trace("[{}] [{}] writing shard snapshot file", shardId, snapshotId); try { + RemoteStorePathStrategy pathStrategy = store.indexSettings().getRemoteStorePathStrategy(); REMOTE_STORE_SHARD_SHALLOW_COPY_SNAPSHOT_FORMAT.write( new RemoteStoreShardShallowCopySnapshot( snapshotId.getName(), @@ -2794,7 +2791,9 @@ public void snapshotRemoteStoreIndexShard( store.indexSettings().getUUID(), store.indexSettings().getRemoteStoreRepository(), this.basePath().toString(), - fileNames + fileNames, + pathStrategy.getType(), + pathStrategy.getHashAlgorithm() ), shardContainer, snapshotId.getUUID(), 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 38670665a9292..553c0a7a85c2f 100644 --- a/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java +++ b/server/src/test/java/org/opensearch/cluster/metadata/MetadataCreateIndexServiceTests.java @@ -1706,12 +1706,9 @@ public void testRemoteCustomData() { // Case 2 - cluster.remote_store.index.path.prefix.optimised=fixed (default value) indexMetadata = testRemoteCustomData(true, PathType.FIXED); - validateRemoteCustomData(indexMetadata.getCustomData(IndexMetadata.REMOTE_STORE_CUSTOM_KEY), PathType.NAME, PathType.FIXED.name()); - validateRemoteCustomData( - indexMetadata.getCustomData(IndexMetadata.REMOTE_STORE_CUSTOM_KEY), - PathHashAlgorithm.NAME, - PathHashAlgorithm.FNV_1A.name() - ); + Map remoteCustomData = indexMetadata.getCustomData(IndexMetadata.REMOTE_STORE_CUSTOM_KEY); + validateRemoteCustomData(remoteCustomData, PathType.NAME, PathType.FIXED.name()); + assertNull(remoteCustomData.get(PathHashAlgorithm.NAME)); // Case 3 - cluster.remote_store.index.path.prefix.optimised=hashed_prefix indexMetadata = testRemoteCustomData(true, PathType.HASHED_PREFIX); diff --git a/server/src/test/java/org/opensearch/index/snapshots/blobstore/RemoteStoreShardShallowCopySnapshotTests.java b/server/src/test/java/org/opensearch/index/snapshots/blobstore/RemoteStoreShardShallowCopySnapshotTests.java index 38c4bb781ce06..e3259a3097278 100644 --- a/server/src/test/java/org/opensearch/index/snapshots/blobstore/RemoteStoreShardShallowCopySnapshotTests.java +++ b/server/src/test/java/org/opensearch/index/snapshots/blobstore/RemoteStoreShardShallowCopySnapshotTests.java @@ -14,6 +14,8 @@ import org.opensearch.core.xcontent.ToXContent; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.index.remote.RemoteStoreEnums.PathHashAlgorithm; +import org.opensearch.index.remote.RemoteStoreEnums.PathType; import org.opensearch.test.OpenSearchTestCase; import java.io.IOException; @@ -40,7 +42,10 @@ public void testToXContent() throws IOException { String repositoryBasePath = "test-repo-basepath"; List fileNames = new ArrayList<>(5); fileNames.addAll(Arrays.asList("file1", "file2", "file3", "file4", "file5")); + + // Case 1 - Without remote path type fields RemoteStoreShardShallowCopySnapshot shardShallowCopySnapshot = new RemoteStoreShardShallowCopySnapshot( + "1", snapshot, indexVersion, primaryTerm, @@ -52,7 +57,9 @@ public void testToXContent() throws IOException { indexUUID, remoteStoreRepository, repositoryBasePath, - fileNames + fileNames, + null, + null ); String actual; try (XContentBuilder builder = MediaTypeRegistry.JSON.contentBuilder()) { @@ -66,6 +73,67 @@ public void testToXContent() throws IOException { + "\"test-rs-repository\",\"commit_generation\":5,\"primary_term\":3,\"remote_store_repository_base_path\":" + "\"test-repo-basepath\",\"file_names\":[\"file1\",\"file2\",\"file3\",\"file4\",\"file5\"]}"; assert Objects.equals(actual, expectedXContent) : "xContent is " + actual; + + // Case 2 - with just fixed type + shardShallowCopySnapshot = new RemoteStoreShardShallowCopySnapshot( + snapshot, + indexVersion, + primaryTerm, + commitGeneration, + startTime, + time, + totalFileCount, + totalSize, + indexUUID, + remoteStoreRepository, + repositoryBasePath, + fileNames, + PathType.FIXED, + null + ); + try (XContentBuilder builder = MediaTypeRegistry.JSON.contentBuilder()) { + builder.startObject(); + shardShallowCopySnapshot.toXContent(builder, ToXContent.EMPTY_PARAMS); + builder.endObject(); + actual = builder.toString(); + } + + expectedXContent = "{\"version\":\"2\",\"name\":\"test-snapshot\",\"index_version\":1,\"start_time\":123,\"time\":123," + + "\"number_of_files\":5,\"total_size\":5,\"index_uuid\":\"syzhajds-ashdlfj\",\"remote_store_repository\":" + + "\"test-rs-repository\",\"commit_generation\":5,\"primary_term\":3,\"remote_store_repository_base_path\":" + + "\"test-repo-basepath\",\"file_names\":[\"file1\",\"file2\",\"file3\",\"file4\",\"file5\"],\"path_type\":0}"; + assert Objects.equals(actual, expectedXContent) : "xContent is " + actual; + + // Case 3 - with just hashed prefix type and hash algorithm + shardShallowCopySnapshot = new RemoteStoreShardShallowCopySnapshot( + snapshot, + indexVersion, + primaryTerm, + commitGeneration, + startTime, + time, + totalFileCount, + totalSize, + indexUUID, + remoteStoreRepository, + repositoryBasePath, + fileNames, + PathType.HASHED_PREFIX, + PathHashAlgorithm.FNV_1A + ); + try (XContentBuilder builder = MediaTypeRegistry.JSON.contentBuilder()) { + builder.startObject(); + shardShallowCopySnapshot.toXContent(builder, ToXContent.EMPTY_PARAMS); + builder.endObject(); + actual = builder.toString(); + } + + expectedXContent = "{\"version\":\"2\",\"name\":\"test-snapshot\",\"index_version\":1,\"start_time\":123,\"time\":123," + + "\"number_of_files\":5,\"total_size\":5,\"index_uuid\":\"syzhajds-ashdlfj\",\"remote_store_repository\":" + + "\"test-rs-repository\",\"commit_generation\":5,\"primary_term\":3,\"remote_store_repository_base_path\":" + + "\"test-repo-basepath\",\"file_names\":[\"file1\",\"file2\",\"file3\",\"file4\",\"file5\"],\"path_type\":1" + + ",\"path_hash_algorithm\":0}"; + assert Objects.equals(actual, expectedXContent) : "xContent is " + actual; } public void testFromXContent() throws IOException { @@ -83,6 +151,7 @@ public void testFromXContent() throws IOException { List fileNames = new ArrayList<>(5); fileNames.addAll(Arrays.asList("file1", "file2", "file3", "file4", "file5")); RemoteStoreShardShallowCopySnapshot expectedShardShallowCopySnapshot = new RemoteStoreShardShallowCopySnapshot( + "1", snapshot, indexVersion, primaryTerm, @@ -94,7 +163,9 @@ public void testFromXContent() throws IOException { indexUUID, remoteStoreRepository, repositoryBasePath, - fileNames + fileNames, + null, + null ); String xContent = "{\"version\":\"1\",\"name\":\"test-snapshot\",\"index_version\":1,\"start_time\":123,\"time\":123," + "\"number_of_files\":5,\"total_size\":5,\"index_uuid\":\"syzhajds-ashdlfj\",\"remote_store_repository\":" @@ -102,22 +173,66 @@ public void testFromXContent() throws IOException { + "\"test-repo-basepath\",\"file_names\":[\"file1\",\"file2\",\"file3\",\"file4\",\"file5\"]}"; try (XContentParser parser = createParser(JsonXContent.jsonXContent, xContent)) { RemoteStoreShardShallowCopySnapshot actualShardShallowCopySnapshot = RemoteStoreShardShallowCopySnapshot.fromXContent(parser); - assertEquals(actualShardShallowCopySnapshot.snapshot(), expectedShardShallowCopySnapshot.snapshot()); - assertEquals( - actualShardShallowCopySnapshot.getRemoteStoreRepository(), - expectedShardShallowCopySnapshot.getRemoteStoreRepository() - ); - assertEquals(actualShardShallowCopySnapshot.getCommitGeneration(), expectedShardShallowCopySnapshot.getCommitGeneration()); - assertEquals(actualShardShallowCopySnapshot.getPrimaryTerm(), expectedShardShallowCopySnapshot.getPrimaryTerm()); - assertEquals(actualShardShallowCopySnapshot.startTime(), expectedShardShallowCopySnapshot.startTime()); - assertEquals(actualShardShallowCopySnapshot.time(), expectedShardShallowCopySnapshot.time()); - assertEquals(actualShardShallowCopySnapshot.totalSize(), expectedShardShallowCopySnapshot.totalSize()); - assertEquals(actualShardShallowCopySnapshot.totalFileCount(), expectedShardShallowCopySnapshot.totalFileCount()); + assert Objects.equals(expectedShardShallowCopySnapshot, actualShardShallowCopySnapshot); + } + + // with pathType=PathType.FIXED + xContent = "{\"version\":\"2\",\"name\":\"test-snapshot\",\"index_version\":1,\"start_time\":123,\"time\":123," + + "\"number_of_files\":5,\"total_size\":5,\"index_uuid\":\"syzhajds-ashdlfj\",\"remote_store_repository\":" + + "\"test-rs-repository\",\"commit_generation\":5,\"primary_term\":3,\"remote_store_repository_base_path\":" + + "\"test-repo-basepath\",\"file_names\":[\"file1\",\"file2\",\"file3\",\"file4\",\"file5\"],\"path_type\":0}"; + expectedShardShallowCopySnapshot = new RemoteStoreShardShallowCopySnapshot( + "2", + snapshot, + indexVersion, + primaryTerm, + commitGeneration, + startTime, + time, + totalFileCount, + totalSize, + indexUUID, + remoteStoreRepository, + repositoryBasePath, + fileNames, + PathType.FIXED, + null + ); + try (XContentParser parser = createParser(JsonXContent.jsonXContent, xContent)) { + RemoteStoreShardShallowCopySnapshot actualShardShallowCopySnapshot = RemoteStoreShardShallowCopySnapshot.fromXContent(parser); + assert Objects.equals(expectedShardShallowCopySnapshot, actualShardShallowCopySnapshot); + } + + // with pathType=PathType.HASHED_PREFIX and pathHashAlgorithm=PathHashAlgorithm.FNV_1A + xContent = "{\"version\":\"2\",\"name\":\"test-snapshot\",\"index_version\":1,\"start_time\":123,\"time\":123," + + "\"number_of_files\":5,\"total_size\":5,\"index_uuid\":\"syzhajds-ashdlfj\",\"remote_store_repository\":" + + "\"test-rs-repository\",\"commit_generation\":5,\"primary_term\":3,\"remote_store_repository_base_path\":" + + "\"test-repo-basepath\",\"file_names\":[\"file1\",\"file2\",\"file3\",\"file4\",\"file5\"],\"path_type\":1,\"path_hash_algorithm\":0}"; + expectedShardShallowCopySnapshot = new RemoteStoreShardShallowCopySnapshot( + "2", + snapshot, + indexVersion, + primaryTerm, + commitGeneration, + startTime, + time, + totalFileCount, + totalSize, + indexUUID, + remoteStoreRepository, + repositoryBasePath, + fileNames, + PathType.HASHED_PREFIX, + PathHashAlgorithm.FNV_1A + ); + try (XContentParser parser = createParser(JsonXContent.jsonXContent, xContent)) { + RemoteStoreShardShallowCopySnapshot actualShardShallowCopySnapshot = RemoteStoreShardShallowCopySnapshot.fromXContent(parser); + assert Objects.equals(expectedShardShallowCopySnapshot, actualShardShallowCopySnapshot); } } public void testFromXContentInvalid() throws IOException { - final int iters = scaledRandomIntBetween(1, 10); + final int iters = 14; for (int iter = 0; iter < iters; iter++) { String snapshot = "test-snapshot"; long indexVersion = 1; @@ -133,10 +248,11 @@ public void testFromXContentInvalid() throws IOException { List fileNames = new ArrayList<>(5); fileNames.addAll(Arrays.asList("file1", "file2", "file3", "file4", "file5")); String failure = null; - String version = RemoteStoreShardShallowCopySnapshot.DEFAULT_VERSION; - long length = Math.max(0, Math.abs(randomLong())); + String version = "1"; + PathType pathType = null; + PathHashAlgorithm pathHashAlgorithm = null; // random corruption - switch (randomIntBetween(0, 8)) { + switch (iter) { case 0: snapshot = null; failure = "Invalid/Missing Snapshot Name"; @@ -170,6 +286,31 @@ public void testFromXContentInvalid() throws IOException { failure = "Invalid Version Provided"; break; case 8: + version = "2"; + failure = "Invalid combination of pathType=null pathHashAlgorithm=null for version=2"; + break; + case 9: + version = "1"; + pathType = PathType.FIXED; + failure = "Invalid combination of pathType=FIXED pathHashAlgorithm=null for version=1"; + break; + case 10: + version = "1"; + pathHashAlgorithm = PathHashAlgorithm.FNV_1A; + failure = "Invalid combination of pathType=null pathHashAlgorithm=FNV_1A for version=1"; + break; + case 11: + version = "2"; + pathType = PathType.FIXED; + pathHashAlgorithm = PathHashAlgorithm.FNV_1A; + failure = "Invalid combination of pathType=FIXED pathHashAlgorithm=FNV_1A for version=2"; + break; + case 12: + version = "2"; + pathType = PathType.HASHED_PREFIX; + pathHashAlgorithm = PathHashAlgorithm.FNV_1A; + break; + case 13: break; default: fail("shouldn't be here"); @@ -194,6 +335,14 @@ public void testFromXContentInvalid() throws IOException { builder.value(fileName); } builder.endArray(); + // We are handling NP check since a cluster can have indexes created earlier which do not have remote store + // path type and path hash algorithm in its custom data in index metadata. + if (Objects.nonNull(pathType)) { + builder.field(RemoteStoreShardShallowCopySnapshot.PATH_TYPE, pathType.getCode()); + } + if (Objects.nonNull(pathHashAlgorithm)) { + builder.field(RemoteStoreShardShallowCopySnapshot.PATH_HASH_ALGORITHM, pathHashAlgorithm.getCode()); + } builder.endObject(); byte[] xContent = BytesReference.toBytes(BytesReference.bytes(builder)); @@ -211,7 +360,8 @@ public void testFromXContentInvalid() throws IOException { assertEquals(remoteStoreShardShallowCopySnapshot.startTime(), startTime); assertEquals(remoteStoreShardShallowCopySnapshot.time(), time); assertEquals(remoteStoreShardShallowCopySnapshot.totalSize(), totalSize); - assertEquals(remoteStoreShardShallowCopySnapshot.totalFileCount(), totalFileCount); + assertEquals(remoteStoreShardShallowCopySnapshot.getPathType(), pathType); + assertEquals(remoteStoreShardShallowCopySnapshot.getPathHashAlgorithm(), pathHashAlgorithm); } else { try (XContentParser parser = createParser(JsonXContent.jsonXContent, xContent)) { parser.nextToken(); diff --git a/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java b/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java index 11b4eb078226f..44ddd2de9d007 100644 --- a/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java +++ b/server/src/test/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryTests.java @@ -704,9 +704,9 @@ public void testCleanupAsync() throws Exception { String repositoryName = "test-repository"; String indexUUID = "test-idx-uuid"; ShardId shardId = new ShardId(Index.UNKNOWN_INDEX_NAME, indexUUID, Integer.parseInt("0")); - RemoteStorePathStrategy pathStrategy = new RemoteStorePathStrategy( - randomFrom(PathType.values()), - randomFrom(PathHashAlgorithm.values()) + RemoteStorePathStrategy pathStrategy = randomFrom( + new RemoteStorePathStrategy(PathType.FIXED), + new RemoteStorePathStrategy(PathType.HASHED_PREFIX, PathHashAlgorithm.FNV_1A) ); RemoteSegmentStoreDirectory.remoteDirectoryCleanup(