hemantk-12 commented on code in PR #4935:
URL: https://github.com/apache/ozone/pull/4935#discussion_r1238989879
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java:
##########
@@ -52,14 +66,61 @@ public OMClientResponse validateAndUpdateCache(OzoneManager
ozoneManager,
SnapshotPurgeRequest snapshotPurgeRequest = getOmRequest()
.getSnapshotPurgeRequest();
- List<String> snapshotDbKeys = snapshotPurgeRequest
- .getSnapshotDBKeysList();
+ try {
+ List<String> snapshotDbKeys = snapshotPurgeRequest
+ .getSnapshotDBKeysList();
+ List<String> snapInfosToUpdate = snapshotPurgeRequest
+ .getUpdatedSnapshotDBKeyList();
+ HashMap<String, SnapshotInfo> updatedSnapInfo = new HashMap<>();
- omClientResponse = new OMSnapshotPurgeResponse(omResponse.build(),
- snapshotDbKeys);
- addResponseToDoubleBuffer(trxnLogIndex, omClientResponse,
- omDoubleBufferHelper);
+ // Snapshots that are already deepCleaned by the KeyDeletingService
+ // can be marked as deepCleaned.
+ for (String snapTableKey : snapInfosToUpdate) {
+ SnapshotInfo snapInfo = omMetadataManager.getSnapshotInfoTable()
+ .get(snapTableKey);
+
+ updateSnapshotInfoAndCache(snapInfo, omMetadataManager,
+ trxnLogIndex, updatedSnapInfo, false);
+ }
+
+ // Snapshots that are purged by the SnapshotDeletingService
+ // will update the next snapshot so that is can be deep cleaned
+ // by the KeyDeletingService in the next run.
+ for (String snapTableKey : snapshotDbKeys) {
+ SnapshotInfo fromSnapshot = omMetadataManager.getSnapshotInfoTable()
+ .get(snapTableKey);
+
+ SnapshotInfo nextSnapshot = SnapshotUtils
+ .getNextActiveSnapshot(fromSnapshot,
+ snapshotChainManager, omSnapshotManager);
+ updateSnapshotInfoAndCache(nextSnapshot, omMetadataManager,
Review Comment:
Is it possible that state change from `true` -> `false` -> `true`?
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java:
##########
@@ -437,6 +445,92 @@ public long optimizeDirDeletesAndSubmitRequest(long
remainNum,
return remainNum;
}
+ protected SnapshotInfo getPreviousActiveSnapshot(SnapshotInfo snapInfo,
+ SnapshotChainManager chainManager, OmSnapshotManager omSnapshotManager)
+ throws IOException {
+ SnapshotInfo currSnapInfo = snapInfo;
+ while (chainManager.hasPreviousPathSnapshot(
+ currSnapInfo.getSnapshotPath(), currSnapInfo.getSnapshotId())) {
+
+ UUID prevPathSnapshot = chainManager.previousPathSnapshot(
+ currSnapInfo.getSnapshotPath(), currSnapInfo.getSnapshotId());
+ String tableKey = chainManager.getTableKey(prevPathSnapshot);
+ SnapshotInfo prevSnapInfo = omSnapshotManager.getSnapshotInfo(tableKey);
+ if (prevSnapInfo.getSnapshotStatus().equals(
+ SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE)) {
+ return prevSnapInfo;
+ }
+ currSnapInfo = prevSnapInfo;
+ }
+ return null;
+ }
+
+ protected boolean isKeyReclaimable(
+ Table<String, OmKeyInfo> previousKeyTable,
+ Table<String, String> renamedTable,
+ OmKeyInfo deletedKeyInfo, OmBucketInfo bucketInfo,
+ long volumeId, HddsProtos.KeyValue.Builder renamedKeyBuilder)
+ throws IOException {
+
+ String dbKey;
+ // Handle case when the deleted snapshot is the first snapshot.
+ if (previousKeyTable == null) {
+ return true;
+ }
+
+ // These are uncommitted blocks wrapped into a pseudo KeyInfo
+ if (deletedKeyInfo.getObjectID() == OBJECT_ID_RECLAIM_BLOCKS) {
+ return true;
+ }
+
+ // Construct keyTable or fileTable DB key depending on the bucket type
+ if (bucketInfo.getBucketLayout().isFileSystemOptimized()) {
+ dbKey = ozoneManager.getMetadataManager().getOzonePathKey(
+ volumeId,
+ bucketInfo.getObjectID(),
+ deletedKeyInfo.getParentObjectID(),
+ deletedKeyInfo.getKeyName());
+ } else {
+ dbKey = ozoneManager.getMetadataManager().getOzoneKey(
+ deletedKeyInfo.getVolumeName(),
+ deletedKeyInfo.getBucketName(),
+ deletedKeyInfo.getKeyName());
+ }
+
+ /*
Review Comment:
alignment is off:
```suggestion
/*
snapshotRenamedTable:
1) /volumeName/bucketName/objectID ->
/volumeId/bucketId/parentId/fileName (FSO)
2) /volumeName/bucketName/objectID ->
/volumeName/bucketName/keyName (non-FSO)
*/
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java:
##########
@@ -52,14 +66,61 @@ public OMClientResponse validateAndUpdateCache(OzoneManager
ozoneManager,
SnapshotPurgeRequest snapshotPurgeRequest = getOmRequest()
.getSnapshotPurgeRequest();
- List<String> snapshotDbKeys = snapshotPurgeRequest
- .getSnapshotDBKeysList();
+ try {
+ List<String> snapshotDbKeys = snapshotPurgeRequest
+ .getSnapshotDBKeysList();
+ List<String> snapInfosToUpdate = snapshotPurgeRequest
+ .getUpdatedSnapshotDBKeyList();
+ HashMap<String, SnapshotInfo> updatedSnapInfo = new HashMap<>();
Review Comment:
nit:
```suggestion
Map<String, SnapshotInfo> updatedSnapInfo = new HashMap<>();
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java:
##########
@@ -48,12 +50,24 @@
public class OMSnapshotPurgeResponse extends OMClientResponse {
private static final Logger LOG =
LoggerFactory.getLogger(OMSnapshotPurgeResponse.class);
- private final List<String> snapshotDbKeys;
+ private List<String> snapshotDbKeys;
+ private HashMap<String, SnapshotInfo> updatedSnapInfo;
public OMSnapshotPurgeResponse(@Nonnull OMResponse omResponse,
- @Nonnull List<String> snapshotDbKeys) {
+ @Nonnull List<String> snapshotDbKeys,
+ HashMap<String, SnapshotInfo> updatedSnapInfo) {
super(omResponse);
this.snapshotDbKeys = snapshotDbKeys;
+ this.updatedSnapInfo = updatedSnapInfo;
+ }
+
+ /**
+ * For when the request is not successful.
Review Comment:
```suggestion
* Constructor for failed request.
* It should not be used for successful request.
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java:
##########
@@ -48,12 +50,24 @@
public class OMSnapshotPurgeResponse extends OMClientResponse {
private static final Logger LOG =
LoggerFactory.getLogger(OMSnapshotPurgeResponse.class);
- private final List<String> snapshotDbKeys;
+ private List<String> snapshotDbKeys;
+ private HashMap<String, SnapshotInfo> updatedSnapInfo;
Review Comment:
nit:
1. Same as other places, use `Map`.
2. `updatedSnapInfo` seems like single instance of SnapshotInfo which got
updated. Please use `updatedSnapInfoMap` or `updatedSnapInfos` to emphasize on
collection.
```suggestion
private Map<String, SnapshotInfo> updatedSnapInfo;
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/snapshot/OMSnapshotPurgeResponse.java:
##########
@@ -48,12 +50,24 @@
public class OMSnapshotPurgeResponse extends OMClientResponse {
private static final Logger LOG =
LoggerFactory.getLogger(OMSnapshotPurgeResponse.class);
- private final List<String> snapshotDbKeys;
+ private List<String> snapshotDbKeys;
+ private HashMap<String, SnapshotInfo> updatedSnapInfo;
Review Comment:
Please make them final and set null (or empty) in other constructor.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/AbstractKeyDeletingService.java:
##########
@@ -437,6 +445,92 @@ public long optimizeDirDeletesAndSubmitRequest(long
remainNum,
return remainNum;
}
+ protected SnapshotInfo getPreviousActiveSnapshot(SnapshotInfo snapInfo,
+ SnapshotChainManager chainManager, OmSnapshotManager omSnapshotManager)
+ throws IOException {
+ SnapshotInfo currSnapInfo = snapInfo;
+ while (chainManager.hasPreviousPathSnapshot(
+ currSnapInfo.getSnapshotPath(), currSnapInfo.getSnapshotId())) {
+
+ UUID prevPathSnapshot = chainManager.previousPathSnapshot(
+ currSnapInfo.getSnapshotPath(), currSnapInfo.getSnapshotId());
+ String tableKey = chainManager.getTableKey(prevPathSnapshot);
+ SnapshotInfo prevSnapInfo = omSnapshotManager.getSnapshotInfo(tableKey);
+ if (prevSnapInfo.getSnapshotStatus().equals(
Review Comment:
nit: use `==` for enum to avoid NPE. Also as a general practice, it is
better to put constant on the left side when `equal` is used to avoid NPE, e.g.
`SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE.equals(prevSnapInfo.getSnapshotStatus())`.
```suggestion
if (prevSnapInfo.getSnapshotStatus() ==
SnapshotInfo.SnapshotStatus.SNAPSHOT_ACTIVE)) {
```
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/snapshot/OMSnapshotPurgeRequest.java:
##########
@@ -52,14 +66,61 @@ public OMClientResponse validateAndUpdateCache(OzoneManager
ozoneManager,
SnapshotPurgeRequest snapshotPurgeRequest = getOmRequest()
.getSnapshotPurgeRequest();
- List<String> snapshotDbKeys = snapshotPurgeRequest
- .getSnapshotDBKeysList();
+ try {
+ List<String> snapshotDbKeys = snapshotPurgeRequest
+ .getSnapshotDBKeysList();
+ List<String> snapInfosToUpdate = snapshotPurgeRequest
+ .getUpdatedSnapshotDBKeyList();
+ HashMap<String, SnapshotInfo> updatedSnapInfo = new HashMap<>();
- omClientResponse = new OMSnapshotPurgeResponse(omResponse.build(),
- snapshotDbKeys);
- addResponseToDoubleBuffer(trxnLogIndex, omClientResponse,
- omDoubleBufferHelper);
+ // Snapshots that are already deepCleaned by the KeyDeletingService
+ // can be marked as deepCleaned.
+ for (String snapTableKey : snapInfosToUpdate) {
+ SnapshotInfo snapInfo = omMetadataManager.getSnapshotInfoTable()
+ .get(snapTableKey);
+
+ updateSnapshotInfoAndCache(snapInfo, omMetadataManager,
+ trxnLogIndex, updatedSnapInfo, false);
+ }
+
+ // Snapshots that are purged by the SnapshotDeletingService
+ // will update the next snapshot so that is can be deep cleaned
+ // by the KeyDeletingService in the next run.
+ for (String snapTableKey : snapshotDbKeys) {
+ SnapshotInfo fromSnapshot = omMetadataManager.getSnapshotInfoTable()
+ .get(snapTableKey);
+
+ SnapshotInfo nextSnapshot = SnapshotUtils
+ .getNextActiveSnapshot(fromSnapshot,
+ snapshotChainManager, omSnapshotManager);
+ updateSnapshotInfoAndCache(nextSnapshot, omMetadataManager,
+ trxnLogIndex, updatedSnapInfo, true);
+ }
+
+ omClientResponse = new OMSnapshotPurgeResponse(omResponse.build(),
+ snapshotDbKeys, updatedSnapInfo);
+ } catch (IOException ex) {
+ omClientResponse = new OMSnapshotPurgeResponse(
+ createErrorOMResponse(omResponse, ex));
+ } finally {
+ addResponseToDoubleBuffer(trxnLogIndex, omClientResponse,
+ omDoubleBufferHelper);
+ }
return omClientResponse;
}
+
+ private void updateSnapshotInfoAndCache(SnapshotInfo snapInfo,
+ OmMetadataManagerImpl omMetadataManager, long trxnLogIndex,
+ HashMap<String, SnapshotInfo> updatedSnapInfo, boolean deepClean) {
Review Comment:
Same as above. Use `Map`.
--
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]
