aswinshakil commented on code in PR #7200:
URL: https://github.com/apache/ozone/pull/7200#discussion_r1764077875
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -89,16 +86,17 @@ public class SnapshotDeletingService extends
AbstractKeyDeletingService {
private final OzoneManager ozoneManager;
private final OmSnapshotManager omSnapshotManager;
- private final SnapshotChainManager chainManager;
+ private final SnapshotChainManager snapshotChainManager;
private final AtomicBoolean suspended;
private final OzoneConfiguration conf;
private final AtomicLong successRunCount;
- private final long snapshotDeletionPerTask;
- private final int keyLimitPerSnapshot;
+ private final int keyLimitPerTask;
+ private final int snapshotDeletionPerTask;
Review Comment:
We should remove `snapshotDeletionPerTask` and process only one snapshot at
a time. Because we might leave many snapshot's halfway moved to next snapshot
since the `keyLimitPerTask` is typically `10000`. It may raise other
inconsistencies when the `nextSnapshot` is deleted after the first move. It
would be better to stick with one snapshot at a time.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/SnapshotDeletingService.java:
##########
@@ -136,316 +134,108 @@ public BackgroundTaskResult call() throws
InterruptedException {
getRunCount().incrementAndGet();
- ReferenceCounted<OmSnapshot> rcOmSnapshot = null;
- ReferenceCounted<OmSnapshot> rcOmPreviousSnapshot = null;
-
- Table<String, SnapshotInfo> snapshotInfoTable =
- ozoneManager.getMetadataManager().getSnapshotInfoTable();
- List<String> purgeSnapshotKeys = new ArrayList<>();
- try (TableIterator<String, ? extends Table.KeyValue
- <String, SnapshotInfo>> iterator = snapshotInfoTable.iterator()) {
-
+ try {
+ int remaining = keyLimitPerTask;
+ Iterator<UUID> iterator = snapshotChainManager.iterator(true);
+ List<SnapshotInfo> snapshotsToBePurged = new ArrayList<>();
long snapshotLimit = snapshotDeletionPerTask;
-
while (iterator.hasNext() && snapshotLimit > 0) {
- SnapshotInfo snapInfo = iterator.next().getValue();
-
- // Only Iterate in deleted snapshot
+ SnapshotInfo snapInfo = SnapshotUtils.getSnapshotInfo(ozoneManager,
snapshotChainManager, iterator.next());
+ // Only Iterate in deleted snapshot & only if all the changes have
been flushed into disk.
if (shouldIgnoreSnapshot(snapInfo)) {
continue;
}
-
- // Note: Can refactor this to use try-with-resources.
- // Handling RC decrements manually for now to minimize conflicts.
- rcOmSnapshot = omSnapshotManager.getSnapshot(
- snapInfo.getVolumeName(),
- snapInfo.getBucketName(),
- snapInfo.getName());
- OmSnapshot omSnapshot = rcOmSnapshot.get();
-
- Table<String, RepeatedOmKeyInfo> snapshotDeletedTable =
- omSnapshot.getMetadataManager().getDeletedTable();
- Table<String, OmKeyInfo> snapshotDeletedDirTable =
- omSnapshot.getMetadataManager().getDeletedDirTable();
-
- Table<String, String> renamedTable =
- omSnapshot.getMetadataManager().getSnapshotRenamedTable();
-
- long volumeId = ozoneManager.getMetadataManager()
- .getVolumeId(snapInfo.getVolumeName());
- // Get bucketInfo for the snapshot bucket to get bucket layout.
- String dbBucketKey = ozoneManager.getMetadataManager().getBucketKey(
- snapInfo.getVolumeName(), snapInfo.getBucketName());
- OmBucketInfo bucketInfo = ozoneManager.getMetadataManager()
- .getBucketTable().get(dbBucketKey);
-
- if (bucketInfo == null) {
- // Decrement ref count
- rcOmSnapshot.close();
- rcOmSnapshot = null;
- throw new IllegalStateException("Bucket " + "/" +
- snapInfo.getVolumeName() + "/" + snapInfo.getBucketName() +
- " is not found. BucketInfo should not be null for snapshotted"
+
- " bucket. The OM is in unexpected state.");
- }
-
- String snapshotBucketKey = dbBucketKey + OzoneConsts.OM_KEY_PREFIX;
- String dbBucketKeyForDir = ozoneManager.getMetadataManager()
- .getBucketKey(Long.toString(volumeId),
- Long.toString(bucketInfo.getObjectID())) + OM_KEY_PREFIX;
-
- if (isSnapshotReclaimable(snapshotDeletedTable,
- snapshotDeletedDirTable, snapshotBucketKey, dbBucketKeyForDir)) {
- purgeSnapshotKeys.add(snapInfo.getTableKey());
- // Decrement ref count
- rcOmSnapshot.close();
- rcOmSnapshot = null;
+ SnapshotInfo nextSnapshot =
SnapshotUtils.getNextSnapshot(ozoneManager, snapshotChainManager, snapInfo);
+ // Continue if the next snapshot is not active. This is to avoid
unnecessary copies from one snapshot to
+ // another.
+ if (nextSnapshot != null &&
+ nextSnapshot.getSnapshotStatus() !=
SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE) {
continue;
}
- //TODO: [SNAPSHOT] Add lock to deletedTable and Active DB.
- SnapshotInfo previousSnapshot = getPreviousActiveSnapshot(snapInfo,
chainManager);
- Table<String, OmKeyInfo> previousKeyTable = null;
- Table<String, OmDirectoryInfo> previousDirTable = null;
- OmSnapshot omPreviousSnapshot = null;
-
- // Split RepeatedOmKeyInfo and update current snapshot
deletedKeyTable
- // and next snapshot deletedKeyTable.
- if (previousSnapshot != null) {
- rcOmPreviousSnapshot = omSnapshotManager.getSnapshot(
- previousSnapshot.getVolumeName(),
- previousSnapshot.getBucketName(),
- previousSnapshot.getName());
- omPreviousSnapshot = rcOmPreviousSnapshot.get();
-
- previousKeyTable = omPreviousSnapshot
-
.getMetadataManager().getKeyTable(bucketInfo.getBucketLayout());
- previousDirTable = omPreviousSnapshot
- .getMetadataManager().getDirectoryTable();
- }
-
- // Move key to either next non deleted snapshot's deletedTable
- // or keep it in current snapshot deleted table.
- List<SnapshotMoveKeyInfos> toReclaimList = new ArrayList<>();
- List<SnapshotMoveKeyInfos> toNextDBList = new ArrayList<>();
- // A list of renamed keys/files/dirs
- List<HddsProtos.KeyValue> renamedList = new ArrayList<>();
- List<String> dirsToMove = new ArrayList<>();
-
- long remainNum = handleDirectoryCleanUp(snapshotDeletedDirTable,
- previousDirTable, renamedTable, dbBucketKeyForDir, snapInfo,
- omSnapshot, dirsToMove, renamedList);
- int deletionCount = 0;
-
- try (TableIterator<String, ? extends Table.KeyValue<String,
- RepeatedOmKeyInfo>> deletedIterator = snapshotDeletedTable
- .iterator()) {
-
- List<BlockGroup> keysToPurge = new ArrayList<>();
- deletedIterator.seek(snapshotBucketKey);
-
- while (deletedIterator.hasNext() &&
- deletionCount < remainNum) {
- Table.KeyValue<String, RepeatedOmKeyInfo>
- deletedKeyValue = deletedIterator.next();
- String deletedKey = deletedKeyValue.getKey();
-
- // Exit if it is out of the bucket scope.
- if (!deletedKey.startsWith(snapshotBucketKey)) {
- // If snapshot deletedKeyTable doesn't have any
- // entry in the snapshot scope it can be reclaimed
- break;
- }
-
- RepeatedOmKeyInfo repeatedOmKeyInfo = deletedKeyValue.getValue();
-
- SnapshotMoveKeyInfos.Builder toReclaim = SnapshotMoveKeyInfos
- .newBuilder()
- .setKey(deletedKey);
- SnapshotMoveKeyInfos.Builder toNextDb = SnapshotMoveKeyInfos
- .newBuilder()
- .setKey(deletedKey);
- HddsProtos.KeyValue.Builder renamedKey = HddsProtos.KeyValue
- .newBuilder();
-
- for (OmKeyInfo keyInfo : repeatedOmKeyInfo.getOmKeyInfoList()) {
- splitRepeatedOmKeyInfo(toReclaim, toNextDb, renamedKey,
- keyInfo, previousKeyTable, renamedTable,
- bucketInfo, volumeId);
+ // nextSnapshot = null means entries would be moved to AOS, hence
ensure that KeyDeletingService &
+ // DirectoryDeletingService is not running while the entries are
moving.
+ if (nextSnapshot == null) {
+ KeyDeletingService keyDeletingService =
getOzoneManager().getKeyManager().getDeletingService();
+ synchronized (keyDeletingService) {
+ while (keyDeletingService != null &&
keyDeletingService.isRunningOnAOS()) {
+ keyDeletingService.wait(serviceTimeout);
}
+ }
- // If all the KeyInfos are reclaimable in RepeatedOmKeyInfo
- // then no need to update current snapshot deletedKeyTable.
- if (!(toReclaim.getKeyInfosCount() ==
- repeatedOmKeyInfo.getOmKeyInfoList().size())) {
- toReclaimList.add(toReclaim.build());
- toNextDBList.add(toNextDb.build());
- } else {
- // The key can be reclaimed here.
- List<BlockGroup> blocksForKeyDelete = omSnapshot
- .getMetadataManager()
- .getBlocksForKeyDelete(deletedKey);
- if (blocksForKeyDelete != null) {
- keysToPurge.addAll(blocksForKeyDelete);
- }
+ DirectoryDeletingService directoryDeletingService =
getOzoneManager().getKeyManager()
+ .getDirDeletingService();
+ synchronized (directoryDeletingService) {
+ while (directoryDeletingService != null &&
directoryDeletingService.isRunningOnAOS()) {
+ directoryDeletingService.wait(serviceTimeout);
}
-
- if (renamedKey.hasKey() && renamedKey.hasValue()) {
- renamedList.add(renamedKey.build());
+ }
+ }
+ try (ReferenceCounted<OmSnapshot> snapshot =
omSnapshotManager.getSnapshot(
+ snapInfo.getVolumeName(), snapInfo.getBucketName(),
snapInfo.getName())) {
+ KeyManager snapshotKeyManager = snapshot.get().getKeyManager();
+ int moveCount = 0;
+ // Get all entries from deletedKeyTable.
+ List<Table.KeyValue<String, List<OmKeyInfo>>> deletedKeyEntries =
+
snapshotKeyManager.getDeletedKeyEntries(snapInfo.getVolumeName(),
snapInfo.getBucketName(),
+ null, remaining);
+ moveCount += deletedKeyEntries.size();
+ // Get all entries from deletedDirTable.
+ List<Table.KeyValue<String, OmKeyInfo>> deletedDirEntries =
snapshotKeyManager.getDeletedDirEntries(
+ snapInfo.getVolumeName(), snapInfo.getBucketName(), remaining
- moveCount);
+ moveCount += deletedDirEntries.size();
+ // Get all entries from snapshotRenamedTable.
+ List<Table.KeyValue<String, String>> renameEntries =
snapshotKeyManager.getRenamesKeyEntries(
+ snapInfo.getVolumeName(), snapInfo.getBucketName(), null,
remaining - moveCount);
+ moveCount += renameEntries.size();
+ if (moveCount > 0) {
+ try {
+ submitSnapshotMoveDeletedKeys(snapInfo,
deletedKeyEntries.stream().map(kv -> {
Review Comment:
We need to simply this function, it has a lot of stream function mapping
which is making it hard to read.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
