Skip to content

Commit

Permalink
Addressed PR comments
Browse files Browse the repository at this point in the history 
Signed-off-by: Bansi Kasundra <kasundra@amazon.com>
  • Loading branch information
@kasundra07
kasundra07 committed Jul 11, 2023
1 parent 8d5aaa1 commit 613d119
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 51 deletions.
7 changes: 6 additions & 1 deletion server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3093,11 +3093,13 @@ private ShardSnapshotMetaDeleteResult deleteFromShardSnapshotMeta(
// Using survivingSnapshots instead of newSnapshotsList as shallow snapshots can be present which won't be part of
// newSnapshotsList
if (survivingSnapshots.isEmpty()) {
// No shallow copy or full copy snapshot is surviving.
return new ShardSnapshotMetaDeleteResult(indexId, snapshotShardId, ShardGenerations.DELETED_SHARD_GEN, blobs);
} else {
final BlobStoreIndexShardSnapshots updatedSnapshots;
// If we have surviving non shallow snapshots, update index- file.
if (newSnapshotsList.size() > 0) {
// Some full copy snapshots are surviving.
updatedSnapshots = new BlobStoreIndexShardSnapshots(newSnapshotsList);
if (indexGeneration < 0L) {
writtenGeneration = UUIDs.randomBase64UUID();
Expand All @@ -3107,8 +3109,11 @@ private ShardSnapshotMetaDeleteResult deleteFromShardSnapshotMeta(
writeShardIndexBlobAtomic(shardContainer, indexGeneration, updatedSnapshots);
}
} else {
// Some shallow copy snapshots are surviving. In this case, since no full copy snapshots are present, we use
// EMPTY BlobStoreIndexShardSnapshots for updatedSnapshots which is used in unusedBlobs to compute stale files,
// and use DELETED_SHARD_GEN since index-N file would not be present anymore.
updatedSnapshots = BlobStoreIndexShardSnapshots.EMPTY;
writtenGeneration = ShardGenerations.NEW_SHARD_GEN;
writtenGeneration = ShardGenerations.DELETED_SHARD_GEN;
}
final Set<String> survivingSnapshotUUIDs = survivingSnapshots.stream().map(SnapshotId::getUUID).collect(Collectors.toSet());
return new ShardSnapshotMetaDeleteResult(
Expand Down
77 changes: 27 additions & 50 deletions server/src/main/java/org/opensearch/snapshots/SnapshotsService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2231,57 +2231,34 @@ private void deleteSnapshotsFromRepository(
final List<SnapshotId> snapshotIds = deleteEntry.getSnapshots();
assert deleteEntry.state() == SnapshotDeletionsInProgress.State.STARTED : "incorrect state for entry [" + deleteEntry + "]";
final Repository repository = repositoriesService.repository(deleteEntry.repository());
StepListener<Boolean> checkForShallowSnapshotStep = new StepListener<Boolean>();

// This check is done to preserve the bwc with repository implementations that don't support shallow snapshot
// and use the existing deleteSnapshots API of Repository plugin. For other repositories, we expect them to implement
// new delete API - deleteSnapshotsAndReleaseLockFiles or use the one implemented by BlobStoreRepository.
// TODO This can be improved by having this information (whether the repository contains any shallow copy snapshot)
// in the RepositoryData instead of fetching snapshot info for each snapshot and verifying.
threadPool.executor(ThreadPool.Names.SNAPSHOT).execute(new AbstractRunnable() {
@Override
public void onFailure(Exception e) {
checkForShallowSnapshotStep.onFailure(e);
}

@Override
protected void doRun() {
boolean cleanupRemoteStoreLockFiles = false;
final List<SnapshotId> allSnapshotIds = repositoryData.getSnapshotIds().stream().collect(Collectors.toList());
for (SnapshotId snapshotId : allSnapshotIds) {
if (Boolean.TRUE.equals(repository.getSnapshotInfo(snapshotId).isRemoteStoreIndexShallowCopyEnabled())) {
cleanupRemoteStoreLockFiles = true;
break;
}
}
checkForShallowSnapshotStep.onResponse(cleanupRemoteStoreLockFiles);
}
});

checkForShallowSnapshotStep.whenComplete(cleanupRemoteStoreLockFiles -> {
if (cleanupRemoteStoreLockFiles) {
repository.deleteSnapshotsAndReleaseLockFiles(
snapshotIds,
repositoryData.getGenId(),
minCompatibleVersion(minNodeVersion, repositoryData, snapshotIds),
remoteStoreLockManagerFactory,
ActionListener.wrap(updatedRepoData -> {
logger.info("snapshots {} deleted", snapshotIds);
removeSnapshotDeletionFromClusterState(deleteEntry, null, updatedRepoData);
}, ex -> removeSnapshotDeletionFromClusterState(deleteEntry, ex, repositoryData))
);
} else {
repository.deleteSnapshots(
snapshotIds,
repositoryData.getGenId(),
minCompatibleVersion(minNodeVersion, repositoryData, snapshotIds),
ActionListener.wrap(updatedRepoData -> {
logger.info("snapshots {} deleted", snapshotIds);
removeSnapshotDeletionFromClusterState(deleteEntry, null, updatedRepoData);
}, ex -> removeSnapshotDeletionFromClusterState(deleteEntry, ex, repositoryData))
);
}
}, ex -> removeSnapshotDeletionFromClusterState(deleteEntry, ex, repositoryData));
// TODO: Relying on repository flag to decide delete flow may lead to shallow snapshot blobs not being taken up for cleanup
// when the repository currently have the flag disabled and we try to delete the shallow snapshots taken prior to disabling
// the flag. This can be improved by having the info whether there ever were any shallow snapshot present in this repository
// or not in RepositoryData.
final boolean cleanupRemoteStoreLockFiles = REMOTE_STORE_INDEX_SHALLOW_COPY.get(repository.getMetadata().settings());
if (cleanupRemoteStoreLockFiles) {
repository.deleteSnapshotsAndReleaseLockFiles(
snapshotIds,
repositoryData.getGenId(),
minCompatibleVersion(minNodeVersion, repositoryData, snapshotIds),
remoteStoreLockManagerFactory,
ActionListener.wrap(updatedRepoData -> {
logger.info("snapshots {} deleted", snapshotIds);
removeSnapshotDeletionFromClusterState(deleteEntry, null, updatedRepoData);
}, ex -> removeSnapshotDeletionFromClusterState(deleteEntry, ex, repositoryData))
);
} else {
repository.deleteSnapshots(
snapshotIds,
repositoryData.getGenId(),
minCompatibleVersion(minNodeVersion, repositoryData, snapshotIds),
ActionListener.wrap(updatedRepoData -> {
logger.info("snapshots {} deleted", snapshotIds);
removeSnapshotDeletionFromClusterState(deleteEntry, null, updatedRepoData);
}, ex -> removeSnapshotDeletionFromClusterState(deleteEntry, ex, repositoryData))
);
}
}
}

Expand Down

0 comments on commit 613d119

Please sign in to comment.