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


##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -140,18 +149,20 @@ public class DirectoryDeletingService extends 
AbstractKeyDeletingService {
   // from parent directory info from deleted directory table concurrently
   // and send deletion requests.
   private int ratisByteLimit;

Review Comment:
   can be made final



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -244,6 +229,71 @@ public void shutdown() {
     super.shutdown();
   }
 
+  @SuppressWarnings("checkstyle:ParameterNumber")
+  void optimizeDirDeletesAndSubmitRequest(
+      long dirNum, long subDirNum, long subFileNum,
+      List<Pair<String, OmKeyInfo>> allSubDirList,
+      List<PurgePathRequest> purgePathRequestList,
+      String snapTableKey, long startTime,
+      long remainingBufLimit, KeyManager keyManager,
+      CheckedFunction<KeyValue<String, OmKeyInfo>, Boolean, IOException> 
reclaimableDirChecker,
+      CheckedFunction<KeyValue<String, OmKeyInfo>, Boolean, IOException> 
reclaimableFileChecker,
+      UUID expectedPreviousSnapshotId, long rnCnt) {
+
+    // Optimization to handle delete sub-dir and keys to remove quickly
+    // This case will be useful to handle when depth of directory is high
+    int subdirDelNum = 0;
+    int subDirRecursiveCnt = 0;
+    int consumedSize = 0;
+    while (subDirRecursiveCnt < allSubDirList.size() && remainingBufLimit > 0) 
{
+      try {
+        Pair<String, OmKeyInfo> stringOmKeyInfoPair = 
allSubDirList.get(subDirRecursiveCnt++);
+        Boolean subDirectoryReclaimable = 
reclaimableDirChecker.apply(Table.newKeyValue(stringOmKeyInfoPair.getKey(),
+            stringOmKeyInfoPair.getValue()));
+        Optional<PurgePathRequest> request = prepareDeleteDirRequest(
+            stringOmKeyInfoPair.getValue(), stringOmKeyInfoPair.getKey(), 
subDirectoryReclaimable, allSubDirList,
+            keyManager, reclaimableFileChecker, remainingBufLimit);
+        if (!request.isPresent()) {
+          continue;
+        }
+        PurgePathRequest requestVal = request.get();
+        consumedSize += requestVal.getSerializedSize();
+        remainingBufLimit -= consumedSize;
+        purgePathRequestList.add(requestVal);
+        // Count up the purgeDeletedDir, subDirs and subFiles
+        if (requestVal.hasDeletedDir() && 
!StringUtils.isBlank(requestVal.getDeletedDir())) {
+          subdirDelNum++;
+        }
+        subDirNum += requestVal.getMarkDeletedSubDirsCount();
+        subFileNum += requestVal.getDeletedSubFilesCount();
+      } catch (IOException e) {
+        LOG.error("Error while running delete directories and files " +
+            "background task. Will retry at next run for subset.", e);
+        break;
+      }
+    }
+    if (!purgePathRequestList.isEmpty()) {
+      submitPurgePaths(purgePathRequestList, snapTableKey, 
expectedPreviousSnapshotId);
+    }
+
+    if (dirNum != 0 || subDirNum != 0 || subFileNum != 0) {
+      long subdirMoved = subDirNum - subdirDelNum;
+      deletedDirsCount.addAndGet(dirNum + subdirDelNum);
+      movedDirsCount.addAndGet(subdirMoved);
+      movedFilesCount.addAndGet(subFileNum);
+      long timeTakenInIteration = Time.monotonicNow() - startTime;
+      LOG.info("Number of dirs deleted: {}, Number of sub-dir " +
+              "deleted: {}, Number of sub-files moved:" +
+              " {} to DeletedTable, Number of sub-dirs moved {} to " +
+              "DeletedDirectoryTable, iteration elapsed: {}ms, " +
+              " totalRunCount: {}",
+          dirNum, subdirDelNum, subFileNum, (subDirNum - subdirDelNum),
+          timeTakenInIteration, rnCnt);
+      getMetrics().incrementDirectoryDeletionTotalMetrics(dirNum + 
subdirDelNum, subDirNum, subFileNum);
+      
getPerfMetrics().setDirectoryDeletingServiceLatencyMs(timeTakenInIteration);
+    }
+  }
+
   private static final class DeletedDirSupplier implements Closeable {
     private TableIterator<String, ? extends KeyValue<String, OmKeyInfo>>
         deleteTableIterator;

Review Comment:
   can be a final variable



##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/service/DirectoryDeletingService.java:
##########
@@ -265,7 +315,159 @@ public void close() {
     }
   }
 
-  private final class DirDeletingTask implements BackgroundTask {
+  /**
+   * Returns the number of dirs deleted by the background service.
+   *
+   * @return Long count.
+   */
+  @VisibleForTesting
+  public long getDeletedDirsCount() {
+    return deletedDirsCount.get();
+  }
+
+  /**
+   * Returns the number of sub-dirs deleted by the background service.
+   *
+   * @return Long count.
+   */
+  @VisibleForTesting
+  public long getMovedDirsCount() {
+    return movedDirsCount.get();
+  }
+
+  /**
+   * Returns the number of files moved to DeletedTable by the background
+   * service.
+   *
+   * @return Long count.
+   */
+  @VisibleForTesting
+  public long getMovedFilesCount() {
+    return movedFilesCount.get();
+  }
+
+  private Optional<PurgePathRequest> prepareDeleteDirRequest(
+      OmKeyInfo pendingDeletedDirInfo, String delDirName, boolean purgeDir,
+      List<Pair<String, OmKeyInfo>> subDirList,
+      KeyManager keyManager,
+      CheckedFunction<Table.KeyValue<String, OmKeyInfo>, Boolean, IOException> 
reclaimableFileFilter,
+      long remainingBufLimit) throws IOException {
+    // step-0: Get one pending deleted directory
+    if (LOG.isDebugEnabled()) {
+      LOG.debug("Pending deleted dir name: {}",
+          pendingDeletedDirInfo.getKeyName());
+    }
+
+    final String[] keys = delDirName.split(OM_KEY_PREFIX);
+    final long volumeId = Long.parseLong(keys[1]);
+    final long bucketId = Long.parseLong(keys[2]);
+
+    // step-1: get all sub directories under the deletedDir
+    DeleteKeysResult subDirDeleteResult =
+        keyManager.getPendingDeletionSubDirs(volumeId, bucketId,
+            pendingDeletedDirInfo, keyInfo -> true, remainingBufLimit);
+    List<OmKeyInfo> subDirs = subDirDeleteResult.getKeysToDelete();
+    remainingBufLimit -= subDirDeleteResult.getConsumedSize();
+
+    OMMetadataManager omMetadataManager = keyManager.getMetadataManager();
+    for (OmKeyInfo dirInfo : subDirs) {
+      String ozoneDbKey = omMetadataManager.getOzonePathKey(volumeId,
+          bucketId, dirInfo.getParentObjectID(), dirInfo.getFileName());
+      String ozoneDeleteKey = omMetadataManager.getOzoneDeletePathKey(
+          dirInfo.getObjectID(), ozoneDbKey);
+      subDirList.add(Pair.of(ozoneDeleteKey, dirInfo));
+      LOG.debug("Moved sub dir name: {}", dirInfo.getKeyName());
+    }
+
+    // step-2: get all sub files under the deletedDir
+    // Only remove sub files if the parent directory is going to be deleted or 
can be reclaimed.
+    DeleteKeysResult subFileDeleteResult =
+        keyManager.getPendingDeletionSubFiles(volumeId, bucketId,
+            pendingDeletedDirInfo, keyInfo -> purgeDir || 
reclaimableFileFilter.apply(keyInfo), remainingBufLimit);
+    List<OmKeyInfo> subFiles = subFileDeleteResult.getKeysToDelete();
+
+    if (LOG.isDebugEnabled()) {
+      for (OmKeyInfo fileInfo : subFiles) {
+        LOG.debug("Moved sub file name: {}", fileInfo.getKeyName());
+      }
+    }
+
+    // step-3: If both sub-dirs and sub-files are exhausted under a parent
+    // directory, only then delete the parent.
+    String purgeDeletedDir = purgeDir && subDirDeleteResult.isProcessedKeys() 
&&
+        subFileDeleteResult.isProcessedKeys() ? delDirName :  null;
+    if (purgeDeletedDir == null && subFiles.isEmpty() && subDirs.isEmpty()) {
+      return Optional.empty();
+    }
+    return Optional.of(wrapPurgeRequest(volumeId, bucketId,
+        purgeDeletedDir, subFiles, subDirs));
+  }
+
+  private OzoneManagerProtocolProtos.PurgePathRequest wrapPurgeRequest(
+      final long volumeId,
+      final long bucketId,
+      final String purgeDeletedDir,
+      final List<OmKeyInfo> purgeDeletedFiles,
+      final List<OmKeyInfo> markDirsAsDeleted) {
+    // Put all keys to be purged in a list
+    PurgePathRequest.Builder purgePathsRequest = PurgePathRequest.newBuilder();
+    purgePathsRequest.setVolumeId(volumeId);
+    purgePathsRequest.setBucketId(bucketId);
+
+    if (purgeDeletedDir != null) {
+      purgePathsRequest.setDeletedDir(purgeDeletedDir);
+    }
+
+    for (OmKeyInfo purgeFile : purgeDeletedFiles) {
+      purgePathsRequest.addDeletedSubFiles(
+          purgeFile.getProtobuf(true, ClientVersion.CURRENT_VERSION));
+    }
+
+    // Add these directories to deletedDirTable, so that its sub-paths will be
+    // traversed in next iteration to ensure cleanup all sub-children.
+    for (OmKeyInfo dir : markDirsAsDeleted) {
+      purgePathsRequest.addMarkDeletedSubDirs(
+          dir.getProtobuf(ClientVersion.CURRENT_VERSION));
+    }
+
+    return purgePathsRequest.build();
+  }
+
+  private OzoneManagerProtocolProtos.OMResponse 
submitPurgePaths(List<PurgePathRequest> requests,

Review Comment:
   return value not used.



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