hemantk-12 commented on code in PR #7193:
URL: https://github.com/apache/ozone/pull/7193#discussion_r1772314813
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java:
##########
@@ -23,13 +23,21 @@
import java.util.HashSet;
import java.util.List;
import java.util.Map;
+import java.util.Objects;
import java.util.Set;
+import java.util.UUID;
+
+import com.google.common.collect.Maps;
+import org.apache.commons.compress.utils.Lists;
import org.apache.commons.lang3.tuple.Pair;
import org.apache.hadoop.hdds.utils.TransactionInfo;
import org.apache.hadoop.hdds.utils.db.cache.CacheKey;
import org.apache.hadoop.hdds.utils.db.cache.CacheValue;
import org.apache.hadoop.ozone.OzoneConsts;
import org.apache.hadoop.ozone.om.OMMetrics;
+import org.apache.hadoop.ozone.om.OmMetadataManagerImpl;
+import org.apache.hadoop.ozone.om.SnapshotChainManager;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
Review Comment:
Most of these imports are not used anywhere. Please remove not-used imports.
##########
hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto:
##########
@@ -1401,6 +1403,12 @@ message PurgePathsResponse {
message PurgeDirectoriesRequest {
repeated PurgePathRequest deletedPath = 1;
optional string snapshotTableKey = 2;
+ // previous snapshotID can also be null & this field would be absent in
older requests.
+ optional NullableUUID expectedPreviousSnapshotID = 3;
+}
+
+message NullableUUID {
Review Comment:
Why do we need to wrap UUID? `expectedPreviousSnapshotID` will be null for
the first snapshot in the chain. Isn't it?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java:
##########
@@ -66,19 +76,34 @@ public OMClientResponse validateAndUpdateCache(OzoneManager
ozoneManager, TermIn
List<OzoneManagerProtocolProtos.PurgePathRequest> purgeRequests =
purgeDirsRequest.getDeletedPathList();
-
- SnapshotInfo fromSnapshotInfo = null;
Set<Pair<String, String>> lockSet = new HashSet<>();
Map<Pair<String, String>, OmBucketInfo> volBucketInfoMap = new HashMap<>();
- OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+ OmMetadataManagerImpl omMetadataManager = (OmMetadataManagerImpl)
ozoneManager.getMetadataManager();
Map<String, OmKeyInfo> openKeyInfoMap = new HashMap<>();
-
OMMetrics omMetrics = ozoneManager.getMetrics();
+ OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
+ getOmRequest());
+ final SnapshotInfo fromSnapshotInfo;
try {
- if (fromSnapshot != null) {
- fromSnapshotInfo = SnapshotUtils.getSnapshotInfo(ozoneManager,
fromSnapshot);
+ fromSnapshotInfo = fromSnapshot != null ?
SnapshotUtils.getSnapshotInfo(ozoneManager,
+ fromSnapshot) : null;
+ // Checking if this request is an old request or new one.
+ if (purgeDirsRequest.hasExpectedPreviousSnapshotID()) {
+ // Validating previous snapshot since while purging deletes, a
snapshot create request could make this purge
Review Comment:
IMO, this is redundant. `validatePreviousSnapshotId` should have a comment
with enough details and that should be sufficient. No need to write a duplicate
comment here and in KeyPurgeRequest.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotUtils.java:
##########
@@ -282,4 +320,47 @@ private static boolean
isSameAsLatestOmKeyInfo(List<OmKeyInfo> omKeyInfos,
}
return false;
}
+
+ public static SnapshotInfo getLatestGlobalSnapshotInfo(OzoneManager
ozoneManager,
Review Comment:
I don't see it being used anywhere. Please remove it, if so.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java:
##########
@@ -66,19 +76,34 @@ public OMClientResponse validateAndUpdateCache(OzoneManager
ozoneManager, TermIn
List<OzoneManagerProtocolProtos.PurgePathRequest> purgeRequests =
purgeDirsRequest.getDeletedPathList();
-
- SnapshotInfo fromSnapshotInfo = null;
Set<Pair<String, String>> lockSet = new HashSet<>();
Map<Pair<String, String>, OmBucketInfo> volBucketInfoMap = new HashMap<>();
- OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+ OmMetadataManagerImpl omMetadataManager = (OmMetadataManagerImpl)
ozoneManager.getMetadataManager();
Map<String, OmKeyInfo> openKeyInfoMap = new HashMap<>();
-
OMMetrics omMetrics = ozoneManager.getMetrics();
+ OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
+ getOmRequest());
+ final SnapshotInfo fromSnapshotInfo;
try {
- if (fromSnapshot != null) {
- fromSnapshotInfo = SnapshotUtils.getSnapshotInfo(ozoneManager,
fromSnapshot);
+ fromSnapshotInfo = fromSnapshot != null ?
SnapshotUtils.getSnapshotInfo(ozoneManager,
+ fromSnapshot) : null;
+ // Checking if this request is an old request or new one.
Review Comment:
nit: this comment and the next one are the same. Maybe combine them.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyPurgeRequest.java:
##########
@@ -58,30 +63,46 @@ public OMKeyPurgeRequest(OMRequest omRequest) {
@Override
public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
TermIndex termIndex) {
PurgeKeysRequest purgeKeysRequest = getOmRequest().getPurgeKeysRequest();
- List<DeletedKeys> bucketDeletedKeysList = purgeKeysRequest
- .getDeletedKeysList();
- List<SnapshotMoveKeyInfos> keysToUpdateList = purgeKeysRequest
- .getKeysToUpdateList();
- String fromSnapshot = purgeKeysRequest.hasSnapshotTableKey() ?
- purgeKeysRequest.getSnapshotTableKey() : null;
- List<String> keysToBePurgedList = new ArrayList<>();
+ List<DeletedKeys> bucketDeletedKeysList =
purgeKeysRequest.getDeletedKeysList();
+ List<SnapshotMoveKeyInfos> keysToUpdateList =
purgeKeysRequest.getKeysToUpdateList();
+ String fromSnapshot = purgeKeysRequest.hasSnapshotTableKey() ?
purgeKeysRequest.getSnapshotTableKey() : null;
OmMetadataManagerImpl omMetadataManager = (OmMetadataManagerImpl)
ozoneManager.getMetadataManager();
OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
getOmRequest());
- OMClientResponse omClientResponse = null;
- for (DeletedKeys bucketWithDeleteKeys : bucketDeletedKeysList) {
- for (String deletedKey : bucketWithDeleteKeys.getKeysList()) {
- keysToBePurgedList.add(deletedKey);
+
+ final SnapshotInfo fromSnapshotInfo;
+ try {
+ fromSnapshotInfo = fromSnapshot != null ?
SnapshotUtils.getSnapshotInfo(ozoneManager,
+ fromSnapshot) : null;
+ // Checking if this request is an old request or new one.
+ if (purgeKeysRequest.hasExpectedPreviousSnapshotID()) {
+ // Validating previous snapshot since while purging deletes, a
snapshot create request could make this purge
+ // directory request invalid on AOS since the deletedDirectory would
be in the newly created snapshot. Adding
+ // subdirectories could lead to not being able to reclaim sub-files
and subdirectories since the
+ // file/directory would be present in the newly created snapshot.
+ // Validating previous snapshot can ensure the chain hasn't changed.
Review Comment:
Please update this comment. This is KeyPurgerRequest not dir.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -265,7 +287,11 @@ private boolean previousSnapshotHasDir(
String prevDbKey = prevDirTableDBKey == null ?
metadataManager.getOzoneDeletePathDirKey(key) :
prevDirTableDBKey;
OmDirectoryInfo prevDirInfo = prevDirTable.get(prevDbKey);
- return prevDirInfo != null &&
+ //Check the snapshot chain hasn't changed while the checking the
previous snapshot.
Review Comment:
```suggestion
// Check the snapshot chain hasn't changed while checking the
previous snapshot.
```
Can you please add what we are checking in the previous snapshot?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java:
##########
@@ -1704,6 +1714,14 @@ public PendingKeysDeletion getPendingDeletionKeys(final
int keyCount,
infoList.getOmKeyInfoList().size()) {
keyBlocksList.addAll(blockGroupList);
}
+ SnapshotInfo newPreviousSnapshotInfo =
SnapshotUtils.getLatestSnapshotInfo(bucketInfo.getVolumeName(),
+ bucketInfo.getBucketName(), ozoneManager,
snapshotChainManager);
+ if
(!Objects.equals(Optional.ofNullable(newPreviousSnapshotInfo).map(SnapshotInfo::getSnapshotId),
+
Optional.ofNullable(previousSnapshotInfo).map(SnapshotInfo::getSnapshotId))) {
+ throw new OMException("Snapshot chain has changed while checking
for key reference " +
Review Comment:
```suggestion
throw new OMException("Snapshot chain has changed while
checking for key reference. " +
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotUtils.java:
##########
@@ -178,6 +179,43 @@ public static SnapshotInfo getNextSnapshot(OzoneManager
ozoneManager,
return null;
}
+ /**
+ * Get the previous in the snapshot chain.
+ */
+ public static SnapshotInfo getPreviousSnapshot(OzoneManager ozoneManager,
+ SnapshotChainManager
chainManager,
+ SnapshotInfo snapInfo)
+ throws IOException {
+ UUID previousSnapshotId = getPreviousSnapshotId(snapInfo, chainManager);
+ return previousSnapshotId == null ? null : getSnapshotInfo(ozoneManager,
chainManager, previousSnapshotId);
+ }
+
+ /**
+ * Get the previous in the snapshot chain.
+ */
+ public static UUID getPreviousSnapshotId(SnapshotInfo snapInfo,
Review Comment:
Make it private.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyPurgeRequest.java:
##########
@@ -58,30 +63,46 @@ public OMKeyPurgeRequest(OMRequest omRequest) {
@Override
public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
TermIndex termIndex) {
PurgeKeysRequest purgeKeysRequest = getOmRequest().getPurgeKeysRequest();
- List<DeletedKeys> bucketDeletedKeysList = purgeKeysRequest
- .getDeletedKeysList();
- List<SnapshotMoveKeyInfos> keysToUpdateList = purgeKeysRequest
- .getKeysToUpdateList();
- String fromSnapshot = purgeKeysRequest.hasSnapshotTableKey() ?
- purgeKeysRequest.getSnapshotTableKey() : null;
- List<String> keysToBePurgedList = new ArrayList<>();
+ List<DeletedKeys> bucketDeletedKeysList =
purgeKeysRequest.getDeletedKeysList();
+ List<SnapshotMoveKeyInfos> keysToUpdateList =
purgeKeysRequest.getKeysToUpdateList();
+ String fromSnapshot = purgeKeysRequest.hasSnapshotTableKey() ?
purgeKeysRequest.getSnapshotTableKey() : null;
OmMetadataManagerImpl omMetadataManager = (OmMetadataManagerImpl)
ozoneManager.getMetadataManager();
OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
getOmRequest());
- OMClientResponse omClientResponse = null;
- for (DeletedKeys bucketWithDeleteKeys : bucketDeletedKeysList) {
- for (String deletedKey : bucketWithDeleteKeys.getKeysList()) {
- keysToBePurgedList.add(deletedKey);
+
+ final SnapshotInfo fromSnapshotInfo;
+ try {
+ fromSnapshotInfo = fromSnapshot != null ?
SnapshotUtils.getSnapshotInfo(ozoneManager,
+ fromSnapshot) : null;
+ // Checking if this request is an old request or new one.
+ if (purgeKeysRequest.hasExpectedPreviousSnapshotID()) {
+ // Validating previous snapshot since while purging deletes, a
snapshot create request could make this purge
+ // directory request invalid on AOS since the deletedDirectory would
be in the newly created snapshot. Adding
+ // subdirectories could lead to not being able to reclaim sub-files
and subdirectories since the
+ // file/directory would be present in the newly created snapshot.
+ // Validating previous snapshot can ensure the chain hasn't changed.
+ UUID expectedPreviousSnapshotId =
purgeKeysRequest.getExpectedPreviousSnapshotID().hasUuid()
+ ?
fromProtobuf(purgeKeysRequest.getExpectedPreviousSnapshotID().getUuid()) : null;
+ validatePreviousSnapshotId(fromSnapshotInfo,
omMetadataManager.getSnapshotChainManager(),
+ expectedPreviousSnapshotId);
}
+ } catch (IOException e) {
+ LOG.error("Error occured while performing OMDirectoriesPurge. ", e);
Review Comment:
```suggestion
LOG.error("Error occurred while performing OMDirectoriesPurge. ", e);
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMDirectoriesPurgeRequestWithFSO.java:
##########
@@ -66,19 +76,34 @@ public OMClientResponse validateAndUpdateCache(OzoneManager
ozoneManager, TermIn
List<OzoneManagerProtocolProtos.PurgePathRequest> purgeRequests =
purgeDirsRequest.getDeletedPathList();
-
- SnapshotInfo fromSnapshotInfo = null;
Set<Pair<String, String>> lockSet = new HashSet<>();
Map<Pair<String, String>, OmBucketInfo> volBucketInfoMap = new HashMap<>();
- OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+ OmMetadataManagerImpl omMetadataManager = (OmMetadataManagerImpl)
ozoneManager.getMetadataManager();
Map<String, OmKeyInfo> openKeyInfoMap = new HashMap<>();
-
OMMetrics omMetrics = ozoneManager.getMetrics();
+ OMResponse.Builder omResponse = OmResponseUtil.getOMResponseBuilder(
+ getOmRequest());
+ final SnapshotInfo fromSnapshotInfo;
try {
- if (fromSnapshot != null) {
- fromSnapshotInfo = SnapshotUtils.getSnapshotInfo(ozoneManager,
fromSnapshot);
+ fromSnapshotInfo = fromSnapshot != null ?
SnapshotUtils.getSnapshotInfo(ozoneManager,
+ fromSnapshot) : null;
+ // Checking if this request is an old request or new one.
+ if (purgeDirsRequest.hasExpectedPreviousSnapshotID()) {
+ // Validating previous snapshot since while purging deletes, a
snapshot create request could make this purge
+ // directory request invalid on AOS since the deletedDirectory would
be in the newly created snapshot. Adding
+ // subdirectories could lead to not being able to reclaim sub-files
and subdirectories since the
+ // file/directory would be present in the newly created snapshot.
+ // Validating previous snapshot can ensure the chain hasn't changed.
+ UUID expectedPreviousSnapshotId =
purgeDirsRequest.getExpectedPreviousSnapshotID().hasUuid()
+ ?
fromProtobuf(purgeDirsRequest.getExpectedPreviousSnapshotID().getUuid()) : null;
+ validatePreviousSnapshotId(fromSnapshotInfo,
omMetadataManager.getSnapshotChainManager(),
+ expectedPreviousSnapshotId);
}
-
+ } catch (IOException e) {
+ LOG.error("Error occured while performing OMDirectoriesPurge. ", e);
Review Comment:
```suggestion
LOG.error("Error occurred while performing OMDirectoriesPurge. ", e);
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotUtils.java:
##########
@@ -178,6 +179,43 @@ public static SnapshotInfo getNextSnapshot(OzoneManager
ozoneManager,
return null;
}
+ /**
+ * Get the previous in the snapshot chain.
+ */
+ public static SnapshotInfo getPreviousSnapshot(OzoneManager ozoneManager,
+ SnapshotChainManager
chainManager,
+ SnapshotInfo snapInfo)
+ throws IOException {
+ UUID previousSnapshotId = getPreviousSnapshotId(snapInfo, chainManager);
+ return previousSnapshotId == null ? null : getSnapshotInfo(ozoneManager,
chainManager, previousSnapshotId);
+ }
+
+ /**
+ * Get the previous in the snapshot chain.
+ */
+ public static UUID getPreviousSnapshotId(SnapshotInfo snapInfo,
+ SnapshotChainManager chainManager)
+ throws IOException {
+ // If the snapshot is deleted in the previous run, then the in-memory
+ // SnapshotChainManager might throw NoSuchElementException as the snapshot
+ // is removed in-memory but OMDoubleBuffer has not flushed yet.
+ if (snapInfo == null) {
+ throw new OMException("Snapshot Info is null. Cannot get the next
snapshot", INVALID_SNAPSHOT_ERROR);
+ }
+ try {
+ if (chainManager.hasPreviousPathSnapshot(snapInfo.getSnapshotPath(),
+ snapInfo.getSnapshotId())) {
+ return chainManager.previousPathSnapshot(snapInfo.getSnapshotPath(),
+ snapInfo.getSnapshotId());
+ }
+ } catch (NoSuchElementException ex) {
+ LOG.error("The snapshot {} is not longer in snapshot chain, It " +
Review Comment:
This is redundant. Logged
[here](https://github.com/apache/ozone/blob/8c0b54ef0c661cc4c80f98e34a6615f1e5855761/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/SnapshotChainManager.java#L542)
already.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotUtils.java:
##########
@@ -282,4 +320,47 @@ private static boolean
isSameAsLatestOmKeyInfo(List<OmKeyInfo> omKeyInfos,
}
return false;
}
+
+ public static SnapshotInfo getLatestGlobalSnapshotInfo(OzoneManager
ozoneManager,
+ SnapshotChainManager
snapshotChainManager) throws IOException {
+ Optional<UUID> latestGlobalSnapshot =
Optional.ofNullable(snapshotChainManager.getLatestGlobalSnapshotId());
+ return latestGlobalSnapshot.isPresent() ? getSnapshotInfo(ozoneManager,
snapshotChainManager,
+ latestGlobalSnapshot.get()) : null;
+ }
+
+ public static SnapshotInfo getLatestSnapshotInfo(String volumeName, String
bucketName,
+ OzoneManager ozoneManager,
+ SnapshotChainManager
snapshotChainManager) throws IOException {
+ Optional<UUID> latestPathSnapshot = Optional.ofNullable(
+ getLatestSnapshotId(volumeName, bucketName, snapshotChainManager));
+ return latestPathSnapshot.isPresent() ?
+ getSnapshotInfo(ozoneManager, snapshotChainManager,
latestPathSnapshot.get()) : null;
+ }
+
+ public static UUID getLatestSnapshotId(String volumeName, String bucketName,
+ SnapshotChainManager
snapshotChainManager) throws IOException {
+ String snapshotPath = volumeName + OM_KEY_PREFIX + bucketName;
+ return snapshotChainManager.getLatestPathSnapshotId(snapshotPath);
+ }
+
+ // Validates previous snapshotId given a snapshotInfo or volumeName &
bucketName. Incase snapshotInfo is null, this
+ // would be considered as AOS and previous snapshot becomes the latest
snapshot in the global snapshot chain.
+ // Would throw OMException if validation fails otherwise function would pass.
+ public static void validatePreviousSnapshotId(SnapshotInfo snapshotInfo,
+ SnapshotChainManager
snapshotChainManager,
+ UUID
expectedPreviousSnapshotId) throws IOException {
+ try {
+ UUID previousSnapshotId = snapshotInfo == null ?
snapshotChainManager.getLatestGlobalSnapshotId() :
+ SnapshotUtils.getPreviousSnapshotId(snapshotInfo,
snapshotChainManager);
+ if (!Objects.equals(expectedPreviousSnapshotId, previousSnapshotId)) {
+ throw new OMException("Snapshot validation failed. Expected previous
snapshotId : " +
+ expectedPreviousSnapshotId + " but was " + previousSnapshotId,
+ OMException.ResultCodes.INVALID_REQUEST);
+ }
+ } catch (IOException e) {
+ LOG.error("Error while validating previous snapshot for snapshot: {}",
+ snapshotInfo == null ? null : snapshotInfo.getName(), e);
+ throw e;
+ }
+ }
Review Comment:
This catch is simply catching the exception thrown at like 356. I would
suggest to remove this catch. Also, this is duplicate logging. You are logging
the same
[OMKeyPurgeRequest](https://github.com/apache/ozone/pull/7193/files#diff-f706c5f3b937e2ef0914b2f3e3d691552d8a651c3acf01c60baef6b3b0a1aff7R92)
and
[OMDirectoriesPurgeRequestWithFSO](https://github.com/apache/ozone/pull/7193/files#diff-9c21e51e11862689181392f4e84c7303e44450cbdb810f7a5dba737ce31f7c33R103).
You can simply remove it. If you still want to log it, move it just before
line 356 and without exception.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/snapshot/SnapshotUtils.java:
##########
@@ -178,6 +179,43 @@ public static SnapshotInfo getNextSnapshot(OzoneManager
ozoneManager,
return null;
}
+ /**
+ * Get the previous in the snapshot chain.
+ */
+ public static SnapshotInfo getPreviousSnapshot(OzoneManager ozoneManager,
+ SnapshotChainManager
chainManager,
+ SnapshotInfo snapInfo)
+ throws IOException {
+ UUID previousSnapshotId = getPreviousSnapshotId(snapInfo, chainManager);
+ return previousSnapshotId == null ? null : getSnapshotInfo(ozoneManager,
chainManager, previousSnapshotId);
+ }
+
+ /**
+ * Get the previous in the snapshot chain.
+ */
+ public static UUID getPreviousSnapshotId(SnapshotInfo snapInfo,
+ SnapshotChainManager chainManager)
+ throws IOException {
+ // If the snapshot is deleted in the previous run, then the in-memory
+ // SnapshotChainManager might throw NoSuchElementException as the snapshot
+ // is removed in-memory but OMDoubleBuffer has not flushed yet.
+ if (snapInfo == null) {
+ throw new OMException("Snapshot Info is null. Cannot get the next
snapshot", INVALID_SNAPSHOT_ERROR);
Review Comment:
```suggestion
throw new OMException("Snapshot Info is null. Cannot get the previous
snapshot", INVALID_SNAPSHOT_ERROR);
```
I don't know if it is a good idea to throw an exception in this case. IMO,
we should return null and caller should handle it.
--
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]
