Skip to content

Commit 4cce608

Browse files
ashking94ashking94
authored
Fix stale index deletion in snapshots for hashed prefix path type (#16617)
Signed-off-by: Ashish Singh <[email protected]>
1 parent 3f18562 commit 4cce608

File tree

3 files changed

+76
-20
lines changed

3 files changed

+76
-20
lines changed

‎server/src/internalClusterTest/java/org/opensearch/snapshots/DeleteSnapshotIT.java

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,37 @@ public class DeleteSnapshotIT extends AbstractSnapshotIntegTestCase {
4545

4646
private static final String REMOTE_REPO_NAME = "remote-store-repo-name";
4747

48+
public void testStaleIndexDeletion() throws Exception {
49+
String indexName1 = ".testindex1";
50+
String repoName = "test-restore-snapshot-repo";
51+
String snapshotName1 = "test-restore-snapshot1";
52+
Path absolutePath = randomRepoPath().toAbsolutePath();
53+
logger.info("Path [{}]", absolutePath);
54+
55+
Client client = client();
56+
// Write a document
57+
String docId = Integer.toString(randomInt());
58+
index(indexName1, "_doc", docId, "value", "expected");
59+
createRepository(repoName, "fs", absolutePath);
60+
61+
logger.info("--> snapshot");
62+
CreateSnapshotResponse createSnapshotResponse = client.admin()
63+
.cluster()
64+
.prepareCreateSnapshot(repoName, snapshotName1)
65+
.setWaitForCompletion(true)
66+
.setIndices(indexName1)
67+
.get();
68+
assertTrue(createSnapshotResponse.getSnapshotInfo().successfulShards() > 0);
69+
assertEquals(createSnapshotResponse.getSnapshotInfo().totalShards(), createSnapshotResponse.getSnapshotInfo().successfulShards());
70+
assertEquals(SnapshotState.SUCCESS, createSnapshotResponse.getSnapshotInfo().state());
71+
72+
assertAcked(startDeleteSnapshot(repoName, snapshotName1).get());
73+
assertBusy(() -> assertEquals(0, RemoteStoreBaseIntegTestCase.getFileCount(absolutePath.resolve(BlobStoreRepository.INDICES_DIR))));
74+
assertBusy(() -> assertEquals(0, RemoteStoreBaseIntegTestCase.getFileCount(absolutePath.resolve(SnapshotShardPaths.DIR))));
75+
// At the end there are 2 files that exists - index-N and index.latest
76+
assertBusy(() -> assertEquals(2, RemoteStoreBaseIntegTestCase.getFileCount(absolutePath)));
77+
}
78+
4879
public void testDeleteSnapshot() throws Exception {
4980
disableRepoConsistencyCheck("Remote store repository is being used in the test");
5081
final Path remoteStoreRepoPath = randomRepoPath();

‎server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java

Lines changed: 34 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2383,11 +2383,23 @@ private List<String> findMatchingShardPaths(String indexId, Map<String, BlobMeta
23832383
* @return An Optional containing the shard path with the highest generation number, or empty if the list is empty
23842384
*/
23852385
private Optional<String> findHighestGenerationShardPaths(List<String> matchingShardPaths) {
2386-
return matchingShardPaths.stream()
2387-
.map(s -> s.split("\\" + SnapshotShardPaths.DELIMITER))
2388-
.sorted((a, b) -> Integer.parseInt(b[2]) - Integer.parseInt(a[2]))
2389-
.map(parts -> String.join(SnapshotShardPaths.DELIMITER, parts))
2390-
.findFirst();
2386+
if (matchingShardPaths.isEmpty()) {
2387+
return Optional.empty();
2388+
}
2389+
2390+
int maxGen = Integer.MIN_VALUE;
2391+
String maxGenShardPath = null;
2392+
2393+
for (String shardPath : matchingShardPaths) {
2394+
String[] parts = shardPath.split("\\" + SnapshotShardPaths.DELIMITER);
2395+
int shardCount = Integer.parseInt(parts[parts.length - 3]);
2396+
if (shardCount > maxGen) {
2397+
maxGen = shardCount;
2398+
maxGenShardPath = shardPath;
2399+
}
2400+
}
2401+
assert maxGenShardPath != null : "Valid maxGenShardPath should be present";
2402+
return Optional.of(maxGenShardPath);
23912403
}
23922404

23932405
/**
@@ -2625,22 +2637,28 @@ public void finalizeSnapshot(
26252637
* on account of new indexes by same index name being snapshotted that exists already in the repository's snapshots.
26262638
*/
26272639
private void cleanupRedundantSnapshotShardPaths(Set<String> updatedShardPathsIndexIds) {
2628-
Set<String> updatedIndexIds = updatedShardPathsIndexIds.stream()
2629-
.map(s -> getIndexId(s.split("\\" + SnapshotShardPaths.DELIMITER)[0]))
2630-
.collect(Collectors.toSet());
2631-
Set<String> indexIdShardPaths = getSnapshotShardPaths().keySet();
2632-
List<String> staleShardPaths = indexIdShardPaths.stream().filter(s -> updatedShardPathsIndexIds.contains(s) == false).filter(s -> {
2633-
String indexId = getIndexId(s.split("\\" + SnapshotShardPaths.DELIMITER)[0]);
2634-
return updatedIndexIds.contains(indexId);
2635-
}).collect(Collectors.toList());
26362640
try {
2641+
Set<String> updatedIndexIds = updatedShardPathsIndexIds.stream()
2642+
.map(s -> getIndexId(s.split("\\" + SnapshotShardPaths.DELIMITER)[0]))
2643+
.collect(Collectors.toSet());
2644+
logger.debug(new ParameterizedMessage("updatedIndexIds={}", updatedIndexIds));
2645+
Set<String> indexIdShardPaths = getSnapshotShardPaths().keySet();
2646+
logger.debug(new ParameterizedMessage("indexIdShardPaths={}", indexIdShardPaths));
2647+
List<String> staleShardPaths = indexIdShardPaths.stream()
2648+
.filter(s -> updatedShardPathsIndexIds.contains(s) == false)
2649+
.filter(s -> {
2650+
String indexId = getIndexId(s.split("\\" + SnapshotShardPaths.DELIMITER)[0]);
2651+
return updatedIndexIds.contains(indexId);
2652+
})
2653+
.collect(Collectors.toList());
2654+
logger.debug(new ParameterizedMessage("staleShardPaths={}", staleShardPaths));
26372655
deleteFromContainer(snapshotShardPathBlobContainer(), staleShardPaths);
2638-
} catch (IOException e) {
2656+
} catch (Exception e) {
26392657
logger.warn(
26402658
new ParameterizedMessage(
2641-
"Repository [{}] Exception during snapshot stale index deletion {}",
2659+
"Repository [{}] Exception during snapshot stale index deletion for updatedIndexIds {}",
26422660
metadata.name(),
2643-
staleShardPaths
2661+
updatedShardPathsIndexIds
26442662
),
26452663
e
26462664
);

‎server/src/main/java/org/opensearch/snapshots/SnapshotShardPaths.java

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -92,18 +92,25 @@ public static SnapshotShardPaths fromXContent(XContentParser ignored) {
9292
* Parses a shard path string and extracts relevant shard information.
9393
*
9494
* @param shardPath The shard path string to parse. Expected format is:
95-
* [index_id]#[index_name]#[shard_count]#[path_type_code]#[path_hash_algorithm_code]
95+
* snapshot_path_[index_id].[index_name].[shard_count].[path_type_code].[path_hash_algorithm_code]
9696
* @return A {@link ShardInfo} object containing the parsed index ID and shard count.
9797
* @throws IllegalArgumentException if the shard path format is invalid or cannot be parsed.
9898
*/
9999
public static ShardInfo parseShardPath(String shardPath) {
100100
String[] parts = shardPath.split("\\" + SnapshotShardPaths.DELIMITER);
101-
if (parts.length != 5) {
101+
int len = parts.length;
102+
if (len < 5) {
102103
throw new IllegalArgumentException("Invalid shard path format: " + shardPath);
103104
}
104105
try {
105-
IndexId indexId = new IndexId(parts[1], getIndexId(parts[0]), Integer.parseInt(parts[3]));
106-
int shardCount = Integer.parseInt(parts[2]);
106+
String indexName = shardPath.substring(
107+
// First separator after index id
108+
shardPath.indexOf(DELIMITER) + 1,
109+
// Since we know there are exactly 3 fields at the end
110+
shardPath.lastIndexOf(DELIMITER, shardPath.lastIndexOf(DELIMITER, shardPath.lastIndexOf(DELIMITER) - 1) - 1)
111+
);
112+
IndexId indexId = new IndexId(indexName, getIndexId(parts[0]), Integer.parseInt(parts[len - 2]));
113+
int shardCount = Integer.parseInt(parts[len - 3]);
107114
return new ShardInfo(indexId, shardCount);
108115
} catch (NumberFormatException e) {
109116
throw new IllegalArgumentException("Invalid shard path format: " + shardPath, e);

0 commit comments

Comments
 (0)