jojochuang commented on code in PR #8525:
URL: https://github.com/apache/ozone/pull/8525#discussion_r2116723976


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyDeletingService.java:
##########
@@ -321,6 +321,10 @@ public BackgroundTaskResult call() {
           snapInfo = snapshotId == null ? null :
               SnapshotUtils.getSnapshotInfo(getOzoneManager(), 
snapshotChainManager, snapshotId);
           if (snapInfo != null) {
+            if (snapInfo.getDeepClean()) {
+              LOG.info("Snapshot {} has already been deep cleaned. Skipping 
the snapshot in this iteration.", snapInfo);

Review Comment:
   SnapshotInfo.toString() is a super long and complex string. What about just 
the snapshot id + volume/bucket instead? 



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyDeletingService.java:
##########
@@ -321,6 +321,10 @@ public BackgroundTaskResult call() {
           snapInfo = snapshotId == null ? null :
               SnapshotUtils.getSnapshotInfo(getOzoneManager(), 
snapshotChainManager, snapshotId);
           if (snapInfo != null) {
+            if (snapInfo.getDeepClean()) {

Review Comment:
   would read more natural if it's named isDeepCleaned().



##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestKeyDeletingService.java:
##########
@@ -583,6 +591,41 @@ void testSnapshotDeepClean() throws Exception {
       }
     }
 
+    @Test

Review Comment:
   ```suggestion
       @DisplayName("KeyDeletingService should skip active snapshot retrieval 
for deep cleaned snapshots")
       @Test
   ```



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/KeyDeletingService.java:
##########
@@ -172,11 +172,11 @@ public void setKeyLimitPerTask(int keyLimitPerTask) {
    * the blocks info in its deletedBlockLog), it removes these keys from the
    * DB.
    */
-  private final class KeyDeletingTask implements BackgroundTask {
+  final class KeyDeletingTask implements BackgroundTask {

Review Comment:
   ```suggestion
     @VisibleForTesting
     final class KeyDeletingTask implements BackgroundTask {
   ```



-- 
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]

Reply via email to