[GitHub] [ozone] bharatviswa504 opened a new pull request #2369: HDDS-5349. Avoid usage of locks in listStatus.

[GitBox <gi...@apache.org>]

bharatviswa504 opened a new pull request #2369:
URL: https://github.com/apache/ozone/pull/2369


   ##What changes were proposed in this pull request?
   Avoid usage of locks when iterating rocksdb iterator.
   
   ##What is the link to the Apache JIRA
   https://issues.apache.org/jira/browse/HDDS-5349
   
   ##How was this patch tested?
   Existing UT's.


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

[GitHub] [ozone] bharatviswa504 commented on pull request #2369: HDDS-5349. Avoid usage of locks in listStatus.

[GitBox <gi...@apache.org>]

bharatviswa504 commented on pull request #2369:
URL: https://github.com/apache/ozone/pull/2369#issuecomment-939032267


   Thank You @rakeshadr for the review and offline discussion.


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

[GitHub] [ozone] adoroszlai commented on pull request #2369: HDDS-5349. Avoid usage of locks in listStatus.

[GitBox <gi...@apache.org>]

adoroszlai commented on pull request #2369:
URL: https://github.com/apache/ozone/pull/2369#issuecomment-921012630


   /pending How about moving the BUCKET_LOCK in PREFIX layout


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

[GitHub] [ozone] rakeshadr commented on a change in pull request #2369: HDDS-5349. Avoid usage of locks in listStatus.

[GitBox <gi...@apache.org>]

rakeshadr commented on a change in pull request #2369:
URL: https://github.com/apache/ozone/pull/2369#discussion_r663713965



##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -2318,110 +2319,111 @@ private void listStatusFindKeyInTableCache(
       startKey = OzoneFSUtils.addTrailingSlashIfNeeded(keyName);
     }
 
+    // Note: eliminating the case where startCacheKey could end with '//'
+    String keyArgs = OzoneFSUtils.addTrailingSlashIfNeeded(
+        metadataManager.getOzoneKey(volumeName, bucketName, keyName));
+
     metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,
         bucketName);
+
+    Table keyTable = metadataManager.getKeyTable();
     try {
-      Table keyTable = metadataManager.getKeyTable();
-      Iterator<Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>>>
+      Iterator< Map.Entry< CacheKey< String >, CacheValue< OmKeyInfo > > >
           cacheIter = keyTable.cacheIterator();
       String startCacheKey = OZONE_URI_DELIMITER + volumeName +
           OZONE_URI_DELIMITER + bucketName + OZONE_URI_DELIMITER +
           ((startKey.equals(OZONE_URI_DELIMITER)) ? "" : startKey);
-      // Note: eliminating the case where startCacheKey could end with '//'
-      String keyArgs = OzoneFSUtils.addTrailingSlashIfNeeded(
-          metadataManager.getOzoneKey(volumeName, bucketName, keyName));
 
       // First, find key in TableCache
       listStatusFindKeyInTableCache(cacheIter, keyArgs, startCacheKey,
           recursive, cacheKeyMap, deletedKeySet);
-      // Then, find key in DB
-      String seekKeyInDb =
-          metadataManager.getOzoneKey(volumeName, bucketName, startKey);
-      TableIterator<String, ? extends Table.KeyValue<String, OmKeyInfo>>
-          iterator = keyTable.iterator();
-      iterator.seek(seekKeyInDb);
-      int countEntries = 0;
-      if (iterator.hasNext()) {
-        if (iterator.key().equals(keyArgs)) {
-          // Skip the key itself, since we are listing inside the directory
-          iterator.next();
-        }
-        // Iterate through seek results
-        while (iterator.hasNext() && numEntries - countEntries > 0) {
-          String entryInDb = iterator.key();
-          OmKeyInfo omKeyInfo = iterator.value().getValue();
-          if (entryInDb.startsWith(keyArgs)) {
-            String entryKeyName = omKeyInfo.getKeyName();
-            if (recursive) {
-              // for recursive list all the entries
+    } finally {
+      metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName,
+          bucketName);
+    }
+
+    // Then, find key in DB
+    String seekKeyInDb =
+        metadataManager.getOzoneKey(volumeName, bucketName, startKey);
+    TableIterator<String, ? extends Table.KeyValue<String, OmKeyInfo>>
+        iterator = keyTable.iterator();

Review comment:
       `keyTable.iterator()` is kept outside bucket lock, is that intentional. If yes, can you please add java comments to make it clear to others. 

##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -2483,159 +2484,276 @@ private void listStatusFindKeyInTableCache(
     int countEntries = 0;
 
     // TODO: recursive flag=true will be handled in HDDS-4360 jira.
-    metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,
-            bucketName);
-    try {
-      if (Strings.isNullOrEmpty(startKey)) {
-        OzoneFileStatus fileStatus = getFileStatus(args, clientAddress);
-        if (fileStatus.isFile()) {
-          return Collections.singletonList(fileStatus);
-        }
 
-        // Not required to search in DeletedTable because all the deleted
-        // keys will be marked directly in dirTable or in keyTable by
-        // breaking the pointer to its sub-dirs and sub-files. So, there is no
-        // issue of inconsistency.
-
-        /*
-         * keyName is a directory.
-         * Say, "/a" is the dir name and its objectID is 1024, then seek
-         * will be doing with "1024/" to get all immediate descendants.
-         */
-        if (fileStatus.getKeyInfo() != null) {
-          prefixKeyInDB = fileStatus.getKeyInfo().getObjectID();
-        } else {
-          // list root directory.
-          String bucketKey = metadataManager.getBucketKey(volumeName,
-                  bucketName);
-          OmBucketInfo omBucketInfo =
-                  metadataManager.getBucketTable().get(bucketKey);
-          prefixKeyInDB = omBucketInfo.getObjectID();
-        }
-        seekFileInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
-        seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
-
-        // Order of seek -> (1)Seek files in fileTable (2)Seek dirs in dirTable
-        // 1. Seek the given key in key table.
-        countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
-                prefixPath, prefixKeyInDB, startKey, countEntries, numEntries);
-        // 2. Seek the given key in dir table.
-        getDirectories(cacheDirMap, seekDirInDB, prefixPath, prefixKeyInDB,
-                startKey, countEntries, numEntries, volumeName, bucketName,
-                recursive);
-      } else {
-        /*
-         * startKey will be used in iterator seek and sets the beginning point
-         * for key traversal.
-         * keyName will be used as parentID where the user has requested to
-         * list the keys from.
-         *
-         * When recursive flag=false, parentID won't change between two pages.
-         * For example: OM has a namespace like,
-         *    /a/1...1M files and /a/b/1...1M files.
-         *    /a/1...1M directories and /a/b/1...1M directories.
-         * Listing "/a", will always have the parentID as "a" irrespective of
-         * the startKey value.
-         */
-
-        // Check startKey is an immediate child of keyName. For example,
-        // keyName=/a/ and expected startKey=/a/b. startKey can't be /xyz/b.
-        if (StringUtils.isNotBlank(keyName) &&
-                !OzoneFSUtils.isImmediateChild(keyName, startKey)) {
-          if (LOG.isDebugEnabled()) {
-            LOG.debug("StartKey {} is not an immediate child of keyName {}. " +
-                    "Returns empty list", startKey, keyName);
-          }
-          return Collections.emptyList();
-        }
 
-        // assign startKeyPath if prefixPath is empty string.
-        if (StringUtils.isBlank(prefixPath)) {
-          prefixPath = OzoneFSUtils.getParentDir(startKey);
+    if (Strings.isNullOrEmpty(startKey)) {
+      OzoneFileStatus fileStatus = getFileStatus(args, clientAddress);
+      if (fileStatus.isFile()) {
+        return Collections.singletonList(fileStatus);
+      }
+
+      // Not required to search in DeletedTable because all the deleted
+      // keys will be marked directly in dirTable or in keyTable by
+      // breaking the pointer to its sub-dirs and sub-files. So, there is no
+      // issue of inconsistency.
+
+      /*
+       * keyName is a directory.
+       * Say, "/a" is the dir name and its objectID is 1024, then seek
+       * will be doing with "1024/" to get all immediate descendants.
+       */
+      if (fileStatus.getKeyInfo() != null) {
+        prefixKeyInDB = fileStatus.getKeyInfo().getObjectID();
+      } else {
+        // list root directory.
+        String bucketKey = metadataManager.getBucketKey(volumeName, bucketName);
+        OmBucketInfo omBucketInfo =
+            metadataManager.getBucketTable().get(bucketKey);
+        prefixKeyInDB = omBucketInfo.getObjectID();
+      }
+      seekFileInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
+      seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
+
+      // Order of seek ->
+      // (1)Seek files in fileTable
+      // (2)Seek dirs in dirTable
+
+      Set<String> deletedKeySet = new TreeSet<>();
+      TreeMap<String, OzoneFileStatus> tempCacheDirMap = new TreeMap<>();
+
+      // Get files, dirs and marked for delete entries from cache.
+      countEntries = getFilesAndDirsFromCache(volumeName, bucketName,
+          cacheFileMap, tempCacheDirMap, deletedKeySet, prefixKeyInDB,
+          seekFileInDB, seekDirInDB, prefixPath, startKey, countEntries,
+          numEntries);
+
+      countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
+          prefixPath, prefixKeyInDB, countEntries, numEntries, deletedKeySet);
+
+      // Now first add all entries in the dir cache, if numEntries required
+      // is less than countEntries.
+      for (Map.Entry<String, OzoneFileStatus> dirEntry :
+          tempCacheDirMap.entrySet()) {
+        if (countEntries < numEntries) {
+          cacheDirMap.put(dirEntry.getKey(), dirEntry.getValue());
+          countEntries++;
         }
+      }
 
-        OzoneFileStatus fileStatusInfo = getOzoneFileStatusFSO(volumeName,
-                bucketName, startKey, false, null,
-                args.getLatestVersionLocation(), true);
+      // 2. Seek the given key in dir table.
+      if (countEntries < numEntries) {
+        getDirectories(cacheDirMap, seekDirInDB, prefixPath,
+            prefixKeyInDB, countEntries, numEntries, recursive,
+            volumeName, bucketName, deletedKeySet);
+      }
 
-        if (fileStatusInfo != null) {
-          prefixKeyInDB = fileStatusInfo.getKeyInfo().getParentObjectID();
-          if(fileStatusInfo.isDirectory()){
-            seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB,
-                    fileStatusInfo.getKeyInfo().getFileName());
+    } else {
+      /*
+       * startKey will be used in iterator seek and sets the beginning point
+       * for key traversal.
+       * keyName will be used as parentID where the user has requested to
+       * list the keys from.
+       *
+       * When recursive flag=false, parentID won't change between two pages.
+       * For example: OM has a namespace like,
+       *    /a/1...1M files and /a/b/1...1M files.
+       *    /a/1...1M directories and /a/b/1...1M directories.
+       * Listing "/a", will always have the parentID as "a" irrespective of
+       * the startKey value.
+       */
+
+      // Check startKey is an immediate child of keyName. For example,
+      // keyName=/a/ and expected startKey=/a/b. startKey can't be /xyz/b.
+      if (StringUtils.isNotBlank(keyName) &&
+          !OzoneFSUtils.isImmediateChild(keyName, startKey)) {
+        if (LOG.isDebugEnabled()) {
+          LOG.debug("StartKey {} is not an immediate child of keyName {}. " +
+              "Returns empty list", startKey, keyName);
+        }
+        return Collections.emptyList();
+      }
+
+      // assign startKeyPath if prefixPath is empty string.
+      if (StringUtils.isBlank(prefixPath)) {
+        prefixPath = OzoneFSUtils.getParentDir(startKey);
+      }
+
+      OzoneFileStatus fileStatusInfo = getOzoneFileStatusFSO(volumeName,
+          bucketName, startKey, false, null,
+          args.getLatestVersionLocation(), true);
+
+      if (fileStatusInfo != null) {
+        prefixKeyInDB = fileStatusInfo.getKeyInfo().getParentObjectID();
+        if(fileStatusInfo.isDirectory()){
+          seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB,
+              fileStatusInfo.getKeyInfo().getFileName());
+
+          // Order of seek -> (1) Seek dirs only in dirTable. In OM, always
+          // the order of search is, first seek into fileTable and then
+          // dirTable. So, its not required to search again in the fileTable.
+
+          // Seek the given key in dirTable.
+          metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,
+                bucketName);
+
+          TreeSet<String> deletedKeySet = new TreeSet<>();
+          TreeMap<String, OzoneFileStatus> tempCacheDirMap = new TreeMap<>();
+          try {
+            listStatusFindDirsInTableCache(tempCacheDirMap,
+                metadataManager.getDirectoryTable(),
+                prefixKeyInDB, seekDirInDB, prefixPath, startKey, volumeName,
+                bucketName, countEntries, numEntries, deletedKeySet);
+          } finally {
+            metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName,
+                bucketName);
+          }
 
-            // Order of seek -> (1) Seek dirs only in dirTable. In OM, always
-            // the order of search is, first seek into fileTable and then
-            // dirTable. So, its not required to search again in the fileTable.
+          // Now first add all entries in the dir cache, if numEntries required

Review comment:
       Below logic is duplicated in both if and else blocks. Line-2611 and line-2646. Can we move this outside the if-else block to avoid duplicate?
   
   ```
             for (Map.Entry<String, OzoneFileStatus> dirEntry :
                 tempCacheDirMap.entrySet()) {
               if (countEntries < numEntries) {
                 cacheDirMap.put(dirEntry.getKey(), dirEntry.getValue());
                 countEntries++;
               }
             }
   
             if (countEntries < numEntries) {
               getDirectories(cacheDirMap, seekDirInDB, prefixPath,
                   prefixKeyInDB, countEntries, numEntries, recursive,
                   volumeName, bucketName, deletedKeySet);
             }
   
   ```

##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -2483,159 +2484,276 @@ private void listStatusFindKeyInTableCache(
     int countEntries = 0;
 
     // TODO: recursive flag=true will be handled in HDDS-4360 jira.
-    metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,
-            bucketName);
-    try {
-      if (Strings.isNullOrEmpty(startKey)) {
-        OzoneFileStatus fileStatus = getFileStatus(args, clientAddress);
-        if (fileStatus.isFile()) {
-          return Collections.singletonList(fileStatus);
-        }
 
-        // Not required to search in DeletedTable because all the deleted
-        // keys will be marked directly in dirTable or in keyTable by
-        // breaking the pointer to its sub-dirs and sub-files. So, there is no
-        // issue of inconsistency.
-
-        /*
-         * keyName is a directory.
-         * Say, "/a" is the dir name and its objectID is 1024, then seek
-         * will be doing with "1024/" to get all immediate descendants.
-         */
-        if (fileStatus.getKeyInfo() != null) {
-          prefixKeyInDB = fileStatus.getKeyInfo().getObjectID();
-        } else {
-          // list root directory.
-          String bucketKey = metadataManager.getBucketKey(volumeName,
-                  bucketName);
-          OmBucketInfo omBucketInfo =
-                  metadataManager.getBucketTable().get(bucketKey);
-          prefixKeyInDB = omBucketInfo.getObjectID();
-        }
-        seekFileInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
-        seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
-
-        // Order of seek -> (1)Seek files in fileTable (2)Seek dirs in dirTable
-        // 1. Seek the given key in key table.
-        countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
-                prefixPath, prefixKeyInDB, startKey, countEntries, numEntries);
-        // 2. Seek the given key in dir table.
-        getDirectories(cacheDirMap, seekDirInDB, prefixPath, prefixKeyInDB,
-                startKey, countEntries, numEntries, volumeName, bucketName,
-                recursive);
-      } else {
-        /*
-         * startKey will be used in iterator seek and sets the beginning point
-         * for key traversal.
-         * keyName will be used as parentID where the user has requested to
-         * list the keys from.
-         *
-         * When recursive flag=false, parentID won't change between two pages.
-         * For example: OM has a namespace like,
-         *    /a/1...1M files and /a/b/1...1M files.
-         *    /a/1...1M directories and /a/b/1...1M directories.
-         * Listing "/a", will always have the parentID as "a" irrespective of
-         * the startKey value.
-         */
-
-        // Check startKey is an immediate child of keyName. For example,
-        // keyName=/a/ and expected startKey=/a/b. startKey can't be /xyz/b.
-        if (StringUtils.isNotBlank(keyName) &&
-                !OzoneFSUtils.isImmediateChild(keyName, startKey)) {
-          if (LOG.isDebugEnabled()) {
-            LOG.debug("StartKey {} is not an immediate child of keyName {}. " +
-                    "Returns empty list", startKey, keyName);
-          }
-          return Collections.emptyList();
-        }
 
-        // assign startKeyPath if prefixPath is empty string.
-        if (StringUtils.isBlank(prefixPath)) {
-          prefixPath = OzoneFSUtils.getParentDir(startKey);
+    if (Strings.isNullOrEmpty(startKey)) {
+      OzoneFileStatus fileStatus = getFileStatus(args, clientAddress);
+      if (fileStatus.isFile()) {
+        return Collections.singletonList(fileStatus);
+      }
+
+      // Not required to search in DeletedTable because all the deleted
+      // keys will be marked directly in dirTable or in keyTable by
+      // breaking the pointer to its sub-dirs and sub-files. So, there is no
+      // issue of inconsistency.
+
+      /*
+       * keyName is a directory.
+       * Say, "/a" is the dir name and its objectID is 1024, then seek
+       * will be doing with "1024/" to get all immediate descendants.
+       */
+      if (fileStatus.getKeyInfo() != null) {
+        prefixKeyInDB = fileStatus.getKeyInfo().getObjectID();
+      } else {
+        // list root directory.
+        String bucketKey = metadataManager.getBucketKey(volumeName, bucketName);
+        OmBucketInfo omBucketInfo =
+            metadataManager.getBucketTable().get(bucketKey);
+        prefixKeyInDB = omBucketInfo.getObjectID();
+      }
+      seekFileInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
+      seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
+
+      // Order of seek ->
+      // (1)Seek files in fileTable
+      // (2)Seek dirs in dirTable
+
+      Set<String> deletedKeySet = new TreeSet<>();
+      TreeMap<String, OzoneFileStatus> tempCacheDirMap = new TreeMap<>();
+
+      // Get files, dirs and marked for delete entries from cache.
+      countEntries = getFilesAndDirsFromCache(volumeName, bucketName,
+          cacheFileMap, tempCacheDirMap, deletedKeySet, prefixKeyInDB,
+          seekFileInDB, seekDirInDB, prefixPath, startKey, countEntries,
+          numEntries);
+
+      countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
+          prefixPath, prefixKeyInDB, countEntries, numEntries, deletedKeySet);
+
+      // Now first add all entries in the dir cache, if numEntries required
+      // is less than countEntries.
+      for (Map.Entry<String, OzoneFileStatus> dirEntry :
+          tempCacheDirMap.entrySet()) {
+        if (countEntries < numEntries) {
+          cacheDirMap.put(dirEntry.getKey(), dirEntry.getValue());
+          countEntries++;
         }
+      }
 
-        OzoneFileStatus fileStatusInfo = getOzoneFileStatusFSO(volumeName,
-                bucketName, startKey, false, null,
-                args.getLatestVersionLocation(), true);
+      // 2. Seek the given key in dir table.
+      if (countEntries < numEntries) {
+        getDirectories(cacheDirMap, seekDirInDB, prefixPath,
+            prefixKeyInDB, countEntries, numEntries, recursive,
+            volumeName, bucketName, deletedKeySet);
+      }
 
-        if (fileStatusInfo != null) {
-          prefixKeyInDB = fileStatusInfo.getKeyInfo().getParentObjectID();
-          if(fileStatusInfo.isDirectory()){
-            seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB,
-                    fileStatusInfo.getKeyInfo().getFileName());
+    } else {
+      /*
+       * startKey will be used in iterator seek and sets the beginning point
+       * for key traversal.
+       * keyName will be used as parentID where the user has requested to
+       * list the keys from.
+       *
+       * When recursive flag=false, parentID won't change between two pages.
+       * For example: OM has a namespace like,
+       *    /a/1...1M files and /a/b/1...1M files.
+       *    /a/1...1M directories and /a/b/1...1M directories.
+       * Listing "/a", will always have the parentID as "a" irrespective of
+       * the startKey value.
+       */
+
+      // Check startKey is an immediate child of keyName. For example,
+      // keyName=/a/ and expected startKey=/a/b. startKey can't be /xyz/b.
+      if (StringUtils.isNotBlank(keyName) &&
+          !OzoneFSUtils.isImmediateChild(keyName, startKey)) {
+        if (LOG.isDebugEnabled()) {
+          LOG.debug("StartKey {} is not an immediate child of keyName {}. " +
+              "Returns empty list", startKey, keyName);
+        }
+        return Collections.emptyList();
+      }
+
+      // assign startKeyPath if prefixPath is empty string.
+      if (StringUtils.isBlank(prefixPath)) {
+        prefixPath = OzoneFSUtils.getParentDir(startKey);
+      }
+
+      OzoneFileStatus fileStatusInfo = getOzoneFileStatusFSO(volumeName,
+          bucketName, startKey, false, null,
+          args.getLatestVersionLocation(), true);
+
+      if (fileStatusInfo != null) {
+        prefixKeyInDB = fileStatusInfo.getKeyInfo().getParentObjectID();
+        if(fileStatusInfo.isDirectory()){
+          seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB,
+              fileStatusInfo.getKeyInfo().getFileName());
+
+          // Order of seek -> (1) Seek dirs only in dirTable. In OM, always
+          // the order of search is, first seek into fileTable and then
+          // dirTable. So, its not required to search again in the fileTable.
+
+          // Seek the given key in dirTable.
+          metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,
+                bucketName);
+
+          TreeSet<String> deletedKeySet = new TreeSet<>();
+          TreeMap<String, OzoneFileStatus> tempCacheDirMap = new TreeMap<>();
+          try {
+            listStatusFindDirsInTableCache(tempCacheDirMap,
+                metadataManager.getDirectoryTable(),
+                prefixKeyInDB, seekDirInDB, prefixPath, startKey, volumeName,
+                bucketName, countEntries, numEntries, deletedKeySet);
+          } finally {
+            metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName,
+                bucketName);
+          }
 
-            // Order of seek -> (1) Seek dirs only in dirTable. In OM, always
-            // the order of search is, first seek into fileTable and then
-            // dirTable. So, its not required to search again in the fileTable.
+          // Now first add all entries in the dir cache, if numEntries required
+          // is less than countEntries.
+          for (Map.Entry<String, OzoneFileStatus> dirEntry :
+              tempCacheDirMap.entrySet()) {
+            if (countEntries < numEntries) {
+              cacheDirMap.put(dirEntry.getKey(), dirEntry.getValue());
+              countEntries++;
+            }
+          }
 
-            // Seek the given key in dirTable.
-            getDirectories(cacheDirMap, seekDirInDB, prefixPath,
-                    prefixKeyInDB, startKey, countEntries, numEntries,
-                    volumeName, bucketName, recursive);
-          } else {
-            seekFileInDB = metadataManager.getOzonePathKey(prefixKeyInDB,
-                    fileStatusInfo.getKeyInfo().getFileName());
-            // begins from the first sub-dir under the parent dir
-            seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
-
-            // 1. Seek the given key in key table.
-            countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
-                    prefixPath, prefixKeyInDB, startKey, countEntries,
-                    numEntries);
-            // 2. Seek the given key in dir table.
+          if (countEntries < numEntries) {
             getDirectories(cacheDirMap, seekDirInDB, prefixPath,
-                    prefixKeyInDB, startKey, countEntries, numEntries,
-                    volumeName, bucketName, recursive);
+                prefixKeyInDB, countEntries, numEntries, recursive,
+                volumeName, bucketName, deletedKeySet);
           }
         } else {
-          // TODO: HDDS-4364: startKey can be a non-existed key
-          if (LOG.isDebugEnabled()) {
-            LOG.debug("StartKey {} is a non-existed key and returning empty " +
-                    "list", startKey);
+          seekFileInDB = metadataManager.getOzonePathKey(prefixKeyInDB,
+              fileStatusInfo.getKeyInfo().getFileName());
+          // begins from the first sub-dir under the parent dir
+          seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
+
+          TreeSet<String> deletedKeySet = new TreeSet<>();
+          TreeMap<String, OzoneFileStatus> tempCacheDirMap = new TreeMap<>();
+
+          // Get files, dirs and marked for delete entries from cache.
+          countEntries = getFilesAndDirsFromCache(volumeName, bucketName,
+              cacheFileMap, tempCacheDirMap, deletedKeySet, prefixKeyInDB,
+              seekFileInDB, seekDirInDB, prefixPath, startKey, countEntries,
+              numEntries);
+
+          // 1. Seek the given key in key table.
+          countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
+              prefixPath, prefixKeyInDB, countEntries, numEntries,
+              deletedKeySet);
+
+          // Now first add all entries in the dir cache, if numEntries required
+          // is less than countEntries.
+          for (Map.Entry<String, OzoneFileStatus> dirEntry :
+              tempCacheDirMap.entrySet()) {
+            if (countEntries < numEntries) {
+              cacheDirMap.put(dirEntry.getKey(), dirEntry.getValue());
+              countEntries++;
+            }
+          }
+
+          // 2. Seek the given key in dir table.
+          if (countEntries < numEntries) {
+            getDirectories(cacheDirMap, seekDirInDB, prefixPath,
+                prefixKeyInDB, countEntries, numEntries, recursive,
+                volumeName, bucketName, deletedKeySet);
           }
-          return Collections.emptyList();
         }
+      } else {
+        // TODO: HDDS-4364: startKey can be a non-existed key
+        if (LOG.isDebugEnabled()) {
+          LOG.debug("StartKey {} is a non-existed key and returning empty " +
+                    "list", startKey);
+        }
+        return Collections.emptyList();
       }
-    } finally {
-      metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName,
-              bucketName);
     }
 
+    return buildFinalStatusList(cacheFileMap, cacheDirMap, args, clientAddress);
+
+  }
+
+  /**
+   * Build final OzoneFileStatus list to be returned to client.
+   * @throws IOException
+   */
+  private List<OzoneFileStatus> buildFinalStatusList(
+      Map<String, OzoneFileStatus> cacheFileMap,
+      Map<String, OzoneFileStatus> cacheDirMap, OmKeyArgs omKeyArgs,
+      String clientAddress)
+      throws IOException {
     List<OmKeyInfo> keyInfoList = new ArrayList<>();
+    List<OzoneFileStatus> fileStatusFinalList = new ArrayList<>();
+
     for (OzoneFileStatus fileStatus : cacheFileMap.values()) {
       fileStatusFinalList.add(fileStatus);
       keyInfoList.add(fileStatus.getKeyInfo());
     }
+
     for (OzoneFileStatus fileStatus : cacheDirMap.values()) {
       fileStatusFinalList.add(fileStatus);
     }
-    if (args.getLatestVersionLocation()) {
+
+    if (omKeyArgs.getLatestVersionLocation()) {
       slimLocationVersion(keyInfoList.toArray(new OmKeyInfo[0]));
     }
     // refreshPipeline flag check has been removed as part of
     // https://issues.apache.org/jira/browse/HDDS-3658.
     // Please refer this jira for more details.
     refreshPipeline(keyInfoList);
-    if (args.getSortDatanodes()) {
+
+    if (omKeyArgs.getSortDatanodes()) {
       sortDatanodes(clientAddress, keyInfoList.toArray(new OmKeyInfo[0]));
     }
+
     return fileStatusFinalList;
   }
 
+  /***
+   * Build files, directories and marked for deleted entries from dir/file
+   * cache.
+   */
+  @SuppressWarnings("parameternumber")
+  private int getFilesAndDirsFromCache(String volumeName, String bucketName,

Review comment:
       Can we name the function to reflect bucket_lock behavior, something like, `getFilesAndDirsFromCacheWithBucketLock`




-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

[GitHub] [ozone] bharatviswa504 commented on pull request #2369: HDDS-5349. Avoid usage of locks in listStatus.

[GitBox <gi...@apache.org>]

bharatviswa504 commented on pull request #2369:
URL: https://github.com/apache/ozone/pull/2369#issuecomment-934922956


   @rakeshadr Fixed review comments.
   Can you have a look in to latest changes.


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

[GitHub] [ozone] bharatviswa504 commented on a change in pull request #2369: HDDS-5349. Avoid usage of locks in listStatus.

[GitBox <gi...@apache.org>]

bharatviswa504 commented on a change in pull request #2369:
URL: https://github.com/apache/ozone/pull/2369#discussion_r683236929



##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -2483,159 +2484,276 @@ private void listStatusFindKeyInTableCache(
     int countEntries = 0;
 
     // TODO: recursive flag=true will be handled in HDDS-4360 jira.
-    metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,
-            bucketName);
-    try {
-      if (Strings.isNullOrEmpty(startKey)) {
-        OzoneFileStatus fileStatus = getFileStatus(args, clientAddress);
-        if (fileStatus.isFile()) {
-          return Collections.singletonList(fileStatus);
-        }
 
-        // Not required to search in DeletedTable because all the deleted
-        // keys will be marked directly in dirTable or in keyTable by
-        // breaking the pointer to its sub-dirs and sub-files. So, there is no
-        // issue of inconsistency.
-
-        /*
-         * keyName is a directory.
-         * Say, "/a" is the dir name and its objectID is 1024, then seek
-         * will be doing with "1024/" to get all immediate descendants.
-         */
-        if (fileStatus.getKeyInfo() != null) {
-          prefixKeyInDB = fileStatus.getKeyInfo().getObjectID();
-        } else {
-          // list root directory.
-          String bucketKey = metadataManager.getBucketKey(volumeName,
-                  bucketName);
-          OmBucketInfo omBucketInfo =
-                  metadataManager.getBucketTable().get(bucketKey);
-          prefixKeyInDB = omBucketInfo.getObjectID();
-        }
-        seekFileInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
-        seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
-
-        // Order of seek -> (1)Seek files in fileTable (2)Seek dirs in dirTable
-        // 1. Seek the given key in key table.
-        countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
-                prefixPath, prefixKeyInDB, startKey, countEntries, numEntries);
-        // 2. Seek the given key in dir table.
-        getDirectories(cacheDirMap, seekDirInDB, prefixPath, prefixKeyInDB,
-                startKey, countEntries, numEntries, volumeName, bucketName,
-                recursive);
-      } else {
-        /*
-         * startKey will be used in iterator seek and sets the beginning point
-         * for key traversal.
-         * keyName will be used as parentID where the user has requested to
-         * list the keys from.
-         *
-         * When recursive flag=false, parentID won't change between two pages.
-         * For example: OM has a namespace like,
-         *    /a/1...1M files and /a/b/1...1M files.
-         *    /a/1...1M directories and /a/b/1...1M directories.
-         * Listing "/a", will always have the parentID as "a" irrespective of
-         * the startKey value.
-         */
-
-        // Check startKey is an immediate child of keyName. For example,
-        // keyName=/a/ and expected startKey=/a/b. startKey can't be /xyz/b.
-        if (StringUtils.isNotBlank(keyName) &&
-                !OzoneFSUtils.isImmediateChild(keyName, startKey)) {
-          if (LOG.isDebugEnabled()) {
-            LOG.debug("StartKey {} is not an immediate child of keyName {}. " +
-                    "Returns empty list", startKey, keyName);
-          }
-          return Collections.emptyList();
-        }
 
-        // assign startKeyPath if prefixPath is empty string.
-        if (StringUtils.isBlank(prefixPath)) {
-          prefixPath = OzoneFSUtils.getParentDir(startKey);
+    if (Strings.isNullOrEmpty(startKey)) {
+      OzoneFileStatus fileStatus = getFileStatus(args, clientAddress);
+      if (fileStatus.isFile()) {
+        return Collections.singletonList(fileStatus);
+      }
+
+      // Not required to search in DeletedTable because all the deleted
+      // keys will be marked directly in dirTable or in keyTable by
+      // breaking the pointer to its sub-dirs and sub-files. So, there is no
+      // issue of inconsistency.
+
+      /*
+       * keyName is a directory.
+       * Say, "/a" is the dir name and its objectID is 1024, then seek
+       * will be doing with "1024/" to get all immediate descendants.
+       */
+      if (fileStatus.getKeyInfo() != null) {
+        prefixKeyInDB = fileStatus.getKeyInfo().getObjectID();
+      } else {
+        // list root directory.
+        String bucketKey = metadataManager.getBucketKey(volumeName, bucketName);
+        OmBucketInfo omBucketInfo =
+            metadataManager.getBucketTable().get(bucketKey);
+        prefixKeyInDB = omBucketInfo.getObjectID();
+      }
+      seekFileInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
+      seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
+
+      // Order of seek ->
+      // (1)Seek files in fileTable
+      // (2)Seek dirs in dirTable
+
+      Set<String> deletedKeySet = new TreeSet<>();
+      TreeMap<String, OzoneFileStatus> tempCacheDirMap = new TreeMap<>();
+
+      // Get files, dirs and marked for delete entries from cache.
+      countEntries = getFilesAndDirsFromCache(volumeName, bucketName,
+          cacheFileMap, tempCacheDirMap, deletedKeySet, prefixKeyInDB,
+          seekFileInDB, seekDirInDB, prefixPath, startKey, countEntries,
+          numEntries);
+
+      countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
+          prefixPath, prefixKeyInDB, countEntries, numEntries, deletedKeySet);
+
+      // Now first add all entries in the dir cache, if numEntries required
+      // is less than countEntries.
+      for (Map.Entry<String, OzoneFileStatus> dirEntry :
+          tempCacheDirMap.entrySet()) {
+        if (countEntries < numEntries) {
+          cacheDirMap.put(dirEntry.getKey(), dirEntry.getValue());
+          countEntries++;
         }
+      }
 
-        OzoneFileStatus fileStatusInfo = getOzoneFileStatusFSO(volumeName,
-                bucketName, startKey, false, null,
-                args.getLatestVersionLocation(), true);
+      // 2. Seek the given key in dir table.
+      if (countEntries < numEntries) {
+        getDirectories(cacheDirMap, seekDirInDB, prefixPath,
+            prefixKeyInDB, countEntries, numEntries, recursive,
+            volumeName, bucketName, deletedKeySet);
+      }
 
-        if (fileStatusInfo != null) {
-          prefixKeyInDB = fileStatusInfo.getKeyInfo().getParentObjectID();
-          if(fileStatusInfo.isDirectory()){
-            seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB,
-                    fileStatusInfo.getKeyInfo().getFileName());
+    } else {
+      /*
+       * startKey will be used in iterator seek and sets the beginning point
+       * for key traversal.
+       * keyName will be used as parentID where the user has requested to
+       * list the keys from.
+       *
+       * When recursive flag=false, parentID won't change between two pages.
+       * For example: OM has a namespace like,
+       *    /a/1...1M files and /a/b/1...1M files.
+       *    /a/1...1M directories and /a/b/1...1M directories.
+       * Listing "/a", will always have the parentID as "a" irrespective of
+       * the startKey value.
+       */
+
+      // Check startKey is an immediate child of keyName. For example,
+      // keyName=/a/ and expected startKey=/a/b. startKey can't be /xyz/b.
+      if (StringUtils.isNotBlank(keyName) &&
+          !OzoneFSUtils.isImmediateChild(keyName, startKey)) {
+        if (LOG.isDebugEnabled()) {
+          LOG.debug("StartKey {} is not an immediate child of keyName {}. " +
+              "Returns empty list", startKey, keyName);
+        }
+        return Collections.emptyList();
+      }
+
+      // assign startKeyPath if prefixPath is empty string.
+      if (StringUtils.isBlank(prefixPath)) {
+        prefixPath = OzoneFSUtils.getParentDir(startKey);
+      }
+
+      OzoneFileStatus fileStatusInfo = getOzoneFileStatusFSO(volumeName,
+          bucketName, startKey, false, null,
+          args.getLatestVersionLocation(), true);
+
+      if (fileStatusInfo != null) {
+        prefixKeyInDB = fileStatusInfo.getKeyInfo().getParentObjectID();
+        if(fileStatusInfo.isDirectory()){
+          seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB,
+              fileStatusInfo.getKeyInfo().getFileName());
+
+          // Order of seek -> (1) Seek dirs only in dirTable. In OM, always
+          // the order of search is, first seek into fileTable and then
+          // dirTable. So, its not required to search again in the fileTable.
+
+          // Seek the given key in dirTable.
+          metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,
+                bucketName);
+
+          TreeSet<String> deletedKeySet = new TreeSet<>();
+          TreeMap<String, OzoneFileStatus> tempCacheDirMap = new TreeMap<>();
+          try {
+            listStatusFindDirsInTableCache(tempCacheDirMap,
+                metadataManager.getDirectoryTable(),
+                prefixKeyInDB, seekDirInDB, prefixPath, startKey, volumeName,
+                bucketName, countEntries, numEntries, deletedKeySet);
+          } finally {
+            metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName,
+                bucketName);
+          }
 
-            // Order of seek -> (1) Seek dirs only in dirTable. In OM, always
-            // the order of search is, first seek into fileTable and then
-            // dirTable. So, its not required to search again in the fileTable.
+          // Now first add all entries in the dir cache, if numEntries required
+          // is less than countEntries.
+          for (Map.Entry<String, OzoneFileStatus> dirEntry :
+              tempCacheDirMap.entrySet()) {
+            if (countEntries < numEntries) {
+              cacheDirMap.put(dirEntry.getKey(), dirEntry.getValue());
+              countEntries++;
+            }
+          }
 
-            // Seek the given key in dirTable.
-            getDirectories(cacheDirMap, seekDirInDB, prefixPath,
-                    prefixKeyInDB, startKey, countEntries, numEntries,
-                    volumeName, bucketName, recursive);
-          } else {
-            seekFileInDB = metadataManager.getOzonePathKey(prefixKeyInDB,
-                    fileStatusInfo.getKeyInfo().getFileName());
-            // begins from the first sub-dir under the parent dir
-            seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
-
-            // 1. Seek the given key in key table.
-            countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
-                    prefixPath, prefixKeyInDB, startKey, countEntries,
-                    numEntries);
-            // 2. Seek the given key in dir table.
+          if (countEntries < numEntries) {
             getDirectories(cacheDirMap, seekDirInDB, prefixPath,
-                    prefixKeyInDB, startKey, countEntries, numEntries,
-                    volumeName, bucketName, recursive);
+                prefixKeyInDB, countEntries, numEntries, recursive,
+                volumeName, bucketName, deletedKeySet);
           }
         } else {
-          // TODO: HDDS-4364: startKey can be a non-existed key
-          if (LOG.isDebugEnabled()) {
-            LOG.debug("StartKey {} is a non-existed key and returning empty " +
-                    "list", startKey);
+          seekFileInDB = metadataManager.getOzonePathKey(prefixKeyInDB,
+              fileStatusInfo.getKeyInfo().getFileName());
+          // begins from the first sub-dir under the parent dir
+          seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
+
+          TreeSet<String> deletedKeySet = new TreeSet<>();
+          TreeMap<String, OzoneFileStatus> tempCacheDirMap = new TreeMap<>();
+
+          // Get files, dirs and marked for delete entries from cache.
+          countEntries = getFilesAndDirsFromCache(volumeName, bucketName,
+              cacheFileMap, tempCacheDirMap, deletedKeySet, prefixKeyInDB,
+              seekFileInDB, seekDirInDB, prefixPath, startKey, countEntries,
+              numEntries);
+
+          // 1. Seek the given key in key table.
+          countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
+              prefixPath, prefixKeyInDB, countEntries, numEntries,
+              deletedKeySet);
+
+          // Now first add all entries in the dir cache, if numEntries required
+          // is less than countEntries.
+          for (Map.Entry<String, OzoneFileStatus> dirEntry :
+              tempCacheDirMap.entrySet()) {
+            if (countEntries < numEntries) {
+              cacheDirMap.put(dirEntry.getKey(), dirEntry.getValue());
+              countEntries++;
+            }
+          }
+
+          // 2. Seek the given key in dir table.
+          if (countEntries < numEntries) {
+            getDirectories(cacheDirMap, seekDirInDB, prefixPath,
+                prefixKeyInDB, countEntries, numEntries, recursive,
+                volumeName, bucketName, deletedKeySet);
           }
-          return Collections.emptyList();
         }
+      } else {
+        // TODO: HDDS-4364: startKey can be a non-existed key
+        if (LOG.isDebugEnabled()) {
+          LOG.debug("StartKey {} is a non-existed key and returning empty " +
+                    "list", startKey);
+        }
+        return Collections.emptyList();
       }
-    } finally {
-      metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName,
-              bucketName);
     }
 
+    return buildFinalStatusList(cacheFileMap, cacheDirMap, args, clientAddress);
+
+  }
+
+  /**
+   * Build final OzoneFileStatus list to be returned to client.
+   * @throws IOException
+   */
+  private List<OzoneFileStatus> buildFinalStatusList(
+      Map<String, OzoneFileStatus> cacheFileMap,
+      Map<String, OzoneFileStatus> cacheDirMap, OmKeyArgs omKeyArgs,
+      String clientAddress)
+      throws IOException {
     List<OmKeyInfo> keyInfoList = new ArrayList<>();
+    List<OzoneFileStatus> fileStatusFinalList = new ArrayList<>();
+
     for (OzoneFileStatus fileStatus : cacheFileMap.values()) {
       fileStatusFinalList.add(fileStatus);
       keyInfoList.add(fileStatus.getKeyInfo());
     }
+
     for (OzoneFileStatus fileStatus : cacheDirMap.values()) {
       fileStatusFinalList.add(fileStatus);
     }
-    if (args.getLatestVersionLocation()) {
+
+    if (omKeyArgs.getLatestVersionLocation()) {
       slimLocationVersion(keyInfoList.toArray(new OmKeyInfo[0]));
     }
     // refreshPipeline flag check has been removed as part of
     // https://issues.apache.org/jira/browse/HDDS-3658.
     // Please refer this jira for more details.
     refreshPipeline(keyInfoList);
-    if (args.getSortDatanodes()) {
+
+    if (omKeyArgs.getSortDatanodes()) {
       sortDatanodes(clientAddress, keyInfoList.toArray(new OmKeyInfo[0]));
     }
+
     return fileStatusFinalList;
   }
 
+  /***
+   * Build files, directories and marked for deleted entries from dir/file
+   * cache.
+   */
+  @SuppressWarnings("parameternumber")
+  private int getFilesAndDirsFromCache(String volumeName, String bucketName,

Review comment:
       Done

##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -2483,159 +2484,276 @@ private void listStatusFindKeyInTableCache(
     int countEntries = 0;
 
     // TODO: recursive flag=true will be handled in HDDS-4360 jira.
-    metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,
-            bucketName);
-    try {
-      if (Strings.isNullOrEmpty(startKey)) {
-        OzoneFileStatus fileStatus = getFileStatus(args, clientAddress);
-        if (fileStatus.isFile()) {
-          return Collections.singletonList(fileStatus);
-        }
 
-        // Not required to search in DeletedTable because all the deleted
-        // keys will be marked directly in dirTable or in keyTable by
-        // breaking the pointer to its sub-dirs and sub-files. So, there is no
-        // issue of inconsistency.
-
-        /*
-         * keyName is a directory.
-         * Say, "/a" is the dir name and its objectID is 1024, then seek
-         * will be doing with "1024/" to get all immediate descendants.
-         */
-        if (fileStatus.getKeyInfo() != null) {
-          prefixKeyInDB = fileStatus.getKeyInfo().getObjectID();
-        } else {
-          // list root directory.
-          String bucketKey = metadataManager.getBucketKey(volumeName,
-                  bucketName);
-          OmBucketInfo omBucketInfo =
-                  metadataManager.getBucketTable().get(bucketKey);
-          prefixKeyInDB = omBucketInfo.getObjectID();
-        }
-        seekFileInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
-        seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
-
-        // Order of seek -> (1)Seek files in fileTable (2)Seek dirs in dirTable
-        // 1. Seek the given key in key table.
-        countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
-                prefixPath, prefixKeyInDB, startKey, countEntries, numEntries);
-        // 2. Seek the given key in dir table.
-        getDirectories(cacheDirMap, seekDirInDB, prefixPath, prefixKeyInDB,
-                startKey, countEntries, numEntries, volumeName, bucketName,
-                recursive);
-      } else {
-        /*
-         * startKey will be used in iterator seek and sets the beginning point
-         * for key traversal.
-         * keyName will be used as parentID where the user has requested to
-         * list the keys from.
-         *
-         * When recursive flag=false, parentID won't change between two pages.
-         * For example: OM has a namespace like,
-         *    /a/1...1M files and /a/b/1...1M files.
-         *    /a/1...1M directories and /a/b/1...1M directories.
-         * Listing "/a", will always have the parentID as "a" irrespective of
-         * the startKey value.
-         */
-
-        // Check startKey is an immediate child of keyName. For example,
-        // keyName=/a/ and expected startKey=/a/b. startKey can't be /xyz/b.
-        if (StringUtils.isNotBlank(keyName) &&
-                !OzoneFSUtils.isImmediateChild(keyName, startKey)) {
-          if (LOG.isDebugEnabled()) {
-            LOG.debug("StartKey {} is not an immediate child of keyName {}. " +
-                    "Returns empty list", startKey, keyName);
-          }
-          return Collections.emptyList();
-        }
 
-        // assign startKeyPath if prefixPath is empty string.
-        if (StringUtils.isBlank(prefixPath)) {
-          prefixPath = OzoneFSUtils.getParentDir(startKey);
+    if (Strings.isNullOrEmpty(startKey)) {
+      OzoneFileStatus fileStatus = getFileStatus(args, clientAddress);
+      if (fileStatus.isFile()) {
+        return Collections.singletonList(fileStatus);
+      }
+
+      // Not required to search in DeletedTable because all the deleted
+      // keys will be marked directly in dirTable or in keyTable by
+      // breaking the pointer to its sub-dirs and sub-files. So, there is no
+      // issue of inconsistency.
+
+      /*
+       * keyName is a directory.
+       * Say, "/a" is the dir name and its objectID is 1024, then seek
+       * will be doing with "1024/" to get all immediate descendants.
+       */
+      if (fileStatus.getKeyInfo() != null) {
+        prefixKeyInDB = fileStatus.getKeyInfo().getObjectID();
+      } else {
+        // list root directory.
+        String bucketKey = metadataManager.getBucketKey(volumeName, bucketName);
+        OmBucketInfo omBucketInfo =
+            metadataManager.getBucketTable().get(bucketKey);
+        prefixKeyInDB = omBucketInfo.getObjectID();
+      }
+      seekFileInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
+      seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
+
+      // Order of seek ->
+      // (1)Seek files in fileTable
+      // (2)Seek dirs in dirTable
+
+      Set<String> deletedKeySet = new TreeSet<>();
+      TreeMap<String, OzoneFileStatus> tempCacheDirMap = new TreeMap<>();
+
+      // Get files, dirs and marked for delete entries from cache.
+      countEntries = getFilesAndDirsFromCache(volumeName, bucketName,
+          cacheFileMap, tempCacheDirMap, deletedKeySet, prefixKeyInDB,
+          seekFileInDB, seekDirInDB, prefixPath, startKey, countEntries,
+          numEntries);
+
+      countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
+          prefixPath, prefixKeyInDB, countEntries, numEntries, deletedKeySet);
+
+      // Now first add all entries in the dir cache, if numEntries required
+      // is less than countEntries.
+      for (Map.Entry<String, OzoneFileStatus> dirEntry :
+          tempCacheDirMap.entrySet()) {
+        if (countEntries < numEntries) {
+          cacheDirMap.put(dirEntry.getKey(), dirEntry.getValue());
+          countEntries++;
         }
+      }
 
-        OzoneFileStatus fileStatusInfo = getOzoneFileStatusFSO(volumeName,
-                bucketName, startKey, false, null,
-                args.getLatestVersionLocation(), true);
+      // 2. Seek the given key in dir table.
+      if (countEntries < numEntries) {
+        getDirectories(cacheDirMap, seekDirInDB, prefixPath,
+            prefixKeyInDB, countEntries, numEntries, recursive,
+            volumeName, bucketName, deletedKeySet);
+      }
 
-        if (fileStatusInfo != null) {
-          prefixKeyInDB = fileStatusInfo.getKeyInfo().getParentObjectID();
-          if(fileStatusInfo.isDirectory()){
-            seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB,
-                    fileStatusInfo.getKeyInfo().getFileName());
+    } else {
+      /*
+       * startKey will be used in iterator seek and sets the beginning point
+       * for key traversal.
+       * keyName will be used as parentID where the user has requested to
+       * list the keys from.
+       *
+       * When recursive flag=false, parentID won't change between two pages.
+       * For example: OM has a namespace like,
+       *    /a/1...1M files and /a/b/1...1M files.
+       *    /a/1...1M directories and /a/b/1...1M directories.
+       * Listing "/a", will always have the parentID as "a" irrespective of
+       * the startKey value.
+       */
+
+      // Check startKey is an immediate child of keyName. For example,
+      // keyName=/a/ and expected startKey=/a/b. startKey can't be /xyz/b.
+      if (StringUtils.isNotBlank(keyName) &&
+          !OzoneFSUtils.isImmediateChild(keyName, startKey)) {
+        if (LOG.isDebugEnabled()) {
+          LOG.debug("StartKey {} is not an immediate child of keyName {}. " +
+              "Returns empty list", startKey, keyName);
+        }
+        return Collections.emptyList();
+      }
+
+      // assign startKeyPath if prefixPath is empty string.
+      if (StringUtils.isBlank(prefixPath)) {
+        prefixPath = OzoneFSUtils.getParentDir(startKey);
+      }
+
+      OzoneFileStatus fileStatusInfo = getOzoneFileStatusFSO(volumeName,
+          bucketName, startKey, false, null,
+          args.getLatestVersionLocation(), true);
+
+      if (fileStatusInfo != null) {
+        prefixKeyInDB = fileStatusInfo.getKeyInfo().getParentObjectID();
+        if(fileStatusInfo.isDirectory()){
+          seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB,
+              fileStatusInfo.getKeyInfo().getFileName());
+
+          // Order of seek -> (1) Seek dirs only in dirTable. In OM, always
+          // the order of search is, first seek into fileTable and then
+          // dirTable. So, its not required to search again in the fileTable.
+
+          // Seek the given key in dirTable.
+          metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,
+                bucketName);
+
+          TreeSet<String> deletedKeySet = new TreeSet<>();
+          TreeMap<String, OzoneFileStatus> tempCacheDirMap = new TreeMap<>();
+          try {
+            listStatusFindDirsInTableCache(tempCacheDirMap,
+                metadataManager.getDirectoryTable(),
+                prefixKeyInDB, seekDirInDB, prefixPath, startKey, volumeName,
+                bucketName, countEntries, numEntries, deletedKeySet);
+          } finally {
+            metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName,
+                bucketName);
+          }
 
-            // Order of seek -> (1) Seek dirs only in dirTable. In OM, always
-            // the order of search is, first seek into fileTable and then
-            // dirTable. So, its not required to search again in the fileTable.
+          // Now first add all entries in the dir cache, if numEntries required

Review comment:
       Done

##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -2318,110 +2319,111 @@ private void listStatusFindKeyInTableCache(
       startKey = OzoneFSUtils.addTrailingSlashIfNeeded(keyName);
     }
 
+    // Note: eliminating the case where startCacheKey could end with '//'
+    String keyArgs = OzoneFSUtils.addTrailingSlashIfNeeded(
+        metadataManager.getOzoneKey(volumeName, bucketName, keyName));
+
     metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,
         bucketName);
+
+    Table keyTable = metadataManager.getKeyTable();
     try {
-      Table keyTable = metadataManager.getKeyTable();
-      Iterator<Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>>>
+      Iterator< Map.Entry< CacheKey< String >, CacheValue< OmKeyInfo > > >
           cacheIter = keyTable.cacheIterator();
       String startCacheKey = OZONE_URI_DELIMITER + volumeName +
           OZONE_URI_DELIMITER + bucketName + OZONE_URI_DELIMITER +
           ((startKey.equals(OZONE_URI_DELIMITER)) ? "" : startKey);
-      // Note: eliminating the case where startCacheKey could end with '//'
-      String keyArgs = OzoneFSUtils.addTrailingSlashIfNeeded(
-          metadataManager.getOzoneKey(volumeName, bucketName, keyName));
 
       // First, find key in TableCache
       listStatusFindKeyInTableCache(cacheIter, keyArgs, startCacheKey,
           recursive, cacheKeyMap, deletedKeySet);
-      // Then, find key in DB
-      String seekKeyInDb =
-          metadataManager.getOzoneKey(volumeName, bucketName, startKey);
-      TableIterator<String, ? extends Table.KeyValue<String, OmKeyInfo>>
-          iterator = keyTable.iterator();
-      iterator.seek(seekKeyInDb);
-      int countEntries = 0;
-      if (iterator.hasNext()) {
-        if (iterator.key().equals(keyArgs)) {
-          // Skip the key itself, since we are listing inside the directory
-          iterator.next();
-        }
-        // Iterate through seek results
-        while (iterator.hasNext() && numEntries - countEntries > 0) {
-          String entryInDb = iterator.key();
-          OmKeyInfo omKeyInfo = iterator.value().getValue();
-          if (entryInDb.startsWith(keyArgs)) {
-            String entryKeyName = omKeyInfo.getKeyName();
-            if (recursive) {
-              // for recursive list all the entries
+    } finally {
+      metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName,
+          bucketName);
+    }
+
+    // Then, find key in DB
+    String seekKeyInDb =
+        metadataManager.getOzoneKey(volumeName, bucketName, startKey);
+    TableIterator<String, ? extends Table.KeyValue<String, OmKeyInfo>>
+        iterator = keyTable.iterator();

Review comment:
       moved inside lock




-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

[GitHub] [ozone] bharatviswa504 merged pull request #2369: HDDS-5349. Avoid usage of locks in listStatus.

[GitBox <gi...@apache.org>]

bharatviswa504 merged pull request #2369:
URL: https://github.com/apache/ozone/pull/2369


   


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

[GitHub] [ozone] bharatviswa504 commented on pull request #2369: HDDS-5349. Avoid usage of locks in listStatus.

[GitBox <gi...@apache.org>]

bharatviswa504 commented on pull request #2369:
URL: https://github.com/apache/ozone/pull/2369#issuecomment-934907244


   /ready


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

[GitHub] [ozone] bharatviswa504 commented on a change in pull request #2369: HDDS-5349. Avoid usage of locks in listStatus.

[GitBox <gi...@apache.org>]

bharatviswa504 commented on a change in pull request #2369:
URL: https://github.com/apache/ozone/pull/2369#discussion_r683236929



##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -2483,159 +2484,276 @@ private void listStatusFindKeyInTableCache(
     int countEntries = 0;
 
     // TODO: recursive flag=true will be handled in HDDS-4360 jira.
-    metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,
-            bucketName);
-    try {
-      if (Strings.isNullOrEmpty(startKey)) {
-        OzoneFileStatus fileStatus = getFileStatus(args, clientAddress);
-        if (fileStatus.isFile()) {
-          return Collections.singletonList(fileStatus);
-        }
 
-        // Not required to search in DeletedTable because all the deleted
-        // keys will be marked directly in dirTable or in keyTable by
-        // breaking the pointer to its sub-dirs and sub-files. So, there is no
-        // issue of inconsistency.
-
-        /*
-         * keyName is a directory.
-         * Say, "/a" is the dir name and its objectID is 1024, then seek
-         * will be doing with "1024/" to get all immediate descendants.
-         */
-        if (fileStatus.getKeyInfo() != null) {
-          prefixKeyInDB = fileStatus.getKeyInfo().getObjectID();
-        } else {
-          // list root directory.
-          String bucketKey = metadataManager.getBucketKey(volumeName,
-                  bucketName);
-          OmBucketInfo omBucketInfo =
-                  metadataManager.getBucketTable().get(bucketKey);
-          prefixKeyInDB = omBucketInfo.getObjectID();
-        }
-        seekFileInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
-        seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
-
-        // Order of seek -> (1)Seek files in fileTable (2)Seek dirs in dirTable
-        // 1. Seek the given key in key table.
-        countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
-                prefixPath, prefixKeyInDB, startKey, countEntries, numEntries);
-        // 2. Seek the given key in dir table.
-        getDirectories(cacheDirMap, seekDirInDB, prefixPath, prefixKeyInDB,
-                startKey, countEntries, numEntries, volumeName, bucketName,
-                recursive);
-      } else {
-        /*
-         * startKey will be used in iterator seek and sets the beginning point
-         * for key traversal.
-         * keyName will be used as parentID where the user has requested to
-         * list the keys from.
-         *
-         * When recursive flag=false, parentID won't change between two pages.
-         * For example: OM has a namespace like,
-         *    /a/1...1M files and /a/b/1...1M files.
-         *    /a/1...1M directories and /a/b/1...1M directories.
-         * Listing "/a", will always have the parentID as "a" irrespective of
-         * the startKey value.
-         */
-
-        // Check startKey is an immediate child of keyName. For example,
-        // keyName=/a/ and expected startKey=/a/b. startKey can't be /xyz/b.
-        if (StringUtils.isNotBlank(keyName) &&
-                !OzoneFSUtils.isImmediateChild(keyName, startKey)) {
-          if (LOG.isDebugEnabled()) {
-            LOG.debug("StartKey {} is not an immediate child of keyName {}. " +
-                    "Returns empty list", startKey, keyName);
-          }
-          return Collections.emptyList();
-        }
 
-        // assign startKeyPath if prefixPath is empty string.
-        if (StringUtils.isBlank(prefixPath)) {
-          prefixPath = OzoneFSUtils.getParentDir(startKey);
+    if (Strings.isNullOrEmpty(startKey)) {
+      OzoneFileStatus fileStatus = getFileStatus(args, clientAddress);
+      if (fileStatus.isFile()) {
+        return Collections.singletonList(fileStatus);
+      }
+
+      // Not required to search in DeletedTable because all the deleted
+      // keys will be marked directly in dirTable or in keyTable by
+      // breaking the pointer to its sub-dirs and sub-files. So, there is no
+      // issue of inconsistency.
+
+      /*
+       * keyName is a directory.
+       * Say, "/a" is the dir name and its objectID is 1024, then seek
+       * will be doing with "1024/" to get all immediate descendants.
+       */
+      if (fileStatus.getKeyInfo() != null) {
+        prefixKeyInDB = fileStatus.getKeyInfo().getObjectID();
+      } else {
+        // list root directory.
+        String bucketKey = metadataManager.getBucketKey(volumeName, bucketName);
+        OmBucketInfo omBucketInfo =
+            metadataManager.getBucketTable().get(bucketKey);
+        prefixKeyInDB = omBucketInfo.getObjectID();
+      }
+      seekFileInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
+      seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
+
+      // Order of seek ->
+      // (1)Seek files in fileTable
+      // (2)Seek dirs in dirTable
+
+      Set<String> deletedKeySet = new TreeSet<>();
+      TreeMap<String, OzoneFileStatus> tempCacheDirMap = new TreeMap<>();
+
+      // Get files, dirs and marked for delete entries from cache.
+      countEntries = getFilesAndDirsFromCache(volumeName, bucketName,
+          cacheFileMap, tempCacheDirMap, deletedKeySet, prefixKeyInDB,
+          seekFileInDB, seekDirInDB, prefixPath, startKey, countEntries,
+          numEntries);
+
+      countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
+          prefixPath, prefixKeyInDB, countEntries, numEntries, deletedKeySet);
+
+      // Now first add all entries in the dir cache, if numEntries required
+      // is less than countEntries.
+      for (Map.Entry<String, OzoneFileStatus> dirEntry :
+          tempCacheDirMap.entrySet()) {
+        if (countEntries < numEntries) {
+          cacheDirMap.put(dirEntry.getKey(), dirEntry.getValue());
+          countEntries++;
         }
+      }
 
-        OzoneFileStatus fileStatusInfo = getOzoneFileStatusFSO(volumeName,
-                bucketName, startKey, false, null,
-                args.getLatestVersionLocation(), true);
+      // 2. Seek the given key in dir table.
+      if (countEntries < numEntries) {
+        getDirectories(cacheDirMap, seekDirInDB, prefixPath,
+            prefixKeyInDB, countEntries, numEntries, recursive,
+            volumeName, bucketName, deletedKeySet);
+      }
 
-        if (fileStatusInfo != null) {
-          prefixKeyInDB = fileStatusInfo.getKeyInfo().getParentObjectID();
-          if(fileStatusInfo.isDirectory()){
-            seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB,
-                    fileStatusInfo.getKeyInfo().getFileName());
+    } else {
+      /*
+       * startKey will be used in iterator seek and sets the beginning point
+       * for key traversal.
+       * keyName will be used as parentID where the user has requested to
+       * list the keys from.
+       *
+       * When recursive flag=false, parentID won't change between two pages.
+       * For example: OM has a namespace like,
+       *    /a/1...1M files and /a/b/1...1M files.
+       *    /a/1...1M directories and /a/b/1...1M directories.
+       * Listing "/a", will always have the parentID as "a" irrespective of
+       * the startKey value.
+       */
+
+      // Check startKey is an immediate child of keyName. For example,
+      // keyName=/a/ and expected startKey=/a/b. startKey can't be /xyz/b.
+      if (StringUtils.isNotBlank(keyName) &&
+          !OzoneFSUtils.isImmediateChild(keyName, startKey)) {
+        if (LOG.isDebugEnabled()) {
+          LOG.debug("StartKey {} is not an immediate child of keyName {}. " +
+              "Returns empty list", startKey, keyName);
+        }
+        return Collections.emptyList();
+      }
+
+      // assign startKeyPath if prefixPath is empty string.
+      if (StringUtils.isBlank(prefixPath)) {
+        prefixPath = OzoneFSUtils.getParentDir(startKey);
+      }
+
+      OzoneFileStatus fileStatusInfo = getOzoneFileStatusFSO(volumeName,
+          bucketName, startKey, false, null,
+          args.getLatestVersionLocation(), true);
+
+      if (fileStatusInfo != null) {
+        prefixKeyInDB = fileStatusInfo.getKeyInfo().getParentObjectID();
+        if(fileStatusInfo.isDirectory()){
+          seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB,
+              fileStatusInfo.getKeyInfo().getFileName());
+
+          // Order of seek -> (1) Seek dirs only in dirTable. In OM, always
+          // the order of search is, first seek into fileTable and then
+          // dirTable. So, its not required to search again in the fileTable.
+
+          // Seek the given key in dirTable.
+          metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,
+                bucketName);
+
+          TreeSet<String> deletedKeySet = new TreeSet<>();
+          TreeMap<String, OzoneFileStatus> tempCacheDirMap = new TreeMap<>();
+          try {
+            listStatusFindDirsInTableCache(tempCacheDirMap,
+                metadataManager.getDirectoryTable(),
+                prefixKeyInDB, seekDirInDB, prefixPath, startKey, volumeName,
+                bucketName, countEntries, numEntries, deletedKeySet);
+          } finally {
+            metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName,
+                bucketName);
+          }
 
-            // Order of seek -> (1) Seek dirs only in dirTable. In OM, always
-            // the order of search is, first seek into fileTable and then
-            // dirTable. So, its not required to search again in the fileTable.
+          // Now first add all entries in the dir cache, if numEntries required
+          // is less than countEntries.
+          for (Map.Entry<String, OzoneFileStatus> dirEntry :
+              tempCacheDirMap.entrySet()) {
+            if (countEntries < numEntries) {
+              cacheDirMap.put(dirEntry.getKey(), dirEntry.getValue());
+              countEntries++;
+            }
+          }
 
-            // Seek the given key in dirTable.
-            getDirectories(cacheDirMap, seekDirInDB, prefixPath,
-                    prefixKeyInDB, startKey, countEntries, numEntries,
-                    volumeName, bucketName, recursive);
-          } else {
-            seekFileInDB = metadataManager.getOzonePathKey(prefixKeyInDB,
-                    fileStatusInfo.getKeyInfo().getFileName());
-            // begins from the first sub-dir under the parent dir
-            seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
-
-            // 1. Seek the given key in key table.
-            countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
-                    prefixPath, prefixKeyInDB, startKey, countEntries,
-                    numEntries);
-            // 2. Seek the given key in dir table.
+          if (countEntries < numEntries) {
             getDirectories(cacheDirMap, seekDirInDB, prefixPath,
-                    prefixKeyInDB, startKey, countEntries, numEntries,
-                    volumeName, bucketName, recursive);
+                prefixKeyInDB, countEntries, numEntries, recursive,
+                volumeName, bucketName, deletedKeySet);
           }
         } else {
-          // TODO: HDDS-4364: startKey can be a non-existed key
-          if (LOG.isDebugEnabled()) {
-            LOG.debug("StartKey {} is a non-existed key and returning empty " +
-                    "list", startKey);
+          seekFileInDB = metadataManager.getOzonePathKey(prefixKeyInDB,
+              fileStatusInfo.getKeyInfo().getFileName());
+          // begins from the first sub-dir under the parent dir
+          seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
+
+          TreeSet<String> deletedKeySet = new TreeSet<>();
+          TreeMap<String, OzoneFileStatus> tempCacheDirMap = new TreeMap<>();
+
+          // Get files, dirs and marked for delete entries from cache.
+          countEntries = getFilesAndDirsFromCache(volumeName, bucketName,
+              cacheFileMap, tempCacheDirMap, deletedKeySet, prefixKeyInDB,
+              seekFileInDB, seekDirInDB, prefixPath, startKey, countEntries,
+              numEntries);
+
+          // 1. Seek the given key in key table.
+          countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
+              prefixPath, prefixKeyInDB, countEntries, numEntries,
+              deletedKeySet);
+
+          // Now first add all entries in the dir cache, if numEntries required
+          // is less than countEntries.
+          for (Map.Entry<String, OzoneFileStatus> dirEntry :
+              tempCacheDirMap.entrySet()) {
+            if (countEntries < numEntries) {
+              cacheDirMap.put(dirEntry.getKey(), dirEntry.getValue());
+              countEntries++;
+            }
+          }
+
+          // 2. Seek the given key in dir table.
+          if (countEntries < numEntries) {
+            getDirectories(cacheDirMap, seekDirInDB, prefixPath,
+                prefixKeyInDB, countEntries, numEntries, recursive,
+                volumeName, bucketName, deletedKeySet);
           }
-          return Collections.emptyList();
         }
+      } else {
+        // TODO: HDDS-4364: startKey can be a non-existed key
+        if (LOG.isDebugEnabled()) {
+          LOG.debug("StartKey {} is a non-existed key and returning empty " +
+                    "list", startKey);
+        }
+        return Collections.emptyList();
       }
-    } finally {
-      metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName,
-              bucketName);
     }
 
+    return buildFinalStatusList(cacheFileMap, cacheDirMap, args, clientAddress);
+
+  }
+
+  /**
+   * Build final OzoneFileStatus list to be returned to client.
+   * @throws IOException
+   */
+  private List<OzoneFileStatus> buildFinalStatusList(
+      Map<String, OzoneFileStatus> cacheFileMap,
+      Map<String, OzoneFileStatus> cacheDirMap, OmKeyArgs omKeyArgs,
+      String clientAddress)
+      throws IOException {
     List<OmKeyInfo> keyInfoList = new ArrayList<>();
+    List<OzoneFileStatus> fileStatusFinalList = new ArrayList<>();
+
     for (OzoneFileStatus fileStatus : cacheFileMap.values()) {
       fileStatusFinalList.add(fileStatus);
       keyInfoList.add(fileStatus.getKeyInfo());
     }
+
     for (OzoneFileStatus fileStatus : cacheDirMap.values()) {
       fileStatusFinalList.add(fileStatus);
     }
-    if (args.getLatestVersionLocation()) {
+
+    if (omKeyArgs.getLatestVersionLocation()) {
       slimLocationVersion(keyInfoList.toArray(new OmKeyInfo[0]));
     }
     // refreshPipeline flag check has been removed as part of
     // https://issues.apache.org/jira/browse/HDDS-3658.
     // Please refer this jira for more details.
     refreshPipeline(keyInfoList);
-    if (args.getSortDatanodes()) {
+
+    if (omKeyArgs.getSortDatanodes()) {
       sortDatanodes(clientAddress, keyInfoList.toArray(new OmKeyInfo[0]));
     }
+
     return fileStatusFinalList;
   }
 
+  /***
+   * Build files, directories and marked for deleted entries from dir/file
+   * cache.
+   */
+  @SuppressWarnings("parameternumber")
+  private int getFilesAndDirsFromCache(String volumeName, String bucketName,

Review comment:
       Done

##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -2483,159 +2484,276 @@ private void listStatusFindKeyInTableCache(
     int countEntries = 0;
 
     // TODO: recursive flag=true will be handled in HDDS-4360 jira.
-    metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,
-            bucketName);
-    try {
-      if (Strings.isNullOrEmpty(startKey)) {
-        OzoneFileStatus fileStatus = getFileStatus(args, clientAddress);
-        if (fileStatus.isFile()) {
-          return Collections.singletonList(fileStatus);
-        }
 
-        // Not required to search in DeletedTable because all the deleted
-        // keys will be marked directly in dirTable or in keyTable by
-        // breaking the pointer to its sub-dirs and sub-files. So, there is no
-        // issue of inconsistency.
-
-        /*
-         * keyName is a directory.
-         * Say, "/a" is the dir name and its objectID is 1024, then seek
-         * will be doing with "1024/" to get all immediate descendants.
-         */
-        if (fileStatus.getKeyInfo() != null) {
-          prefixKeyInDB = fileStatus.getKeyInfo().getObjectID();
-        } else {
-          // list root directory.
-          String bucketKey = metadataManager.getBucketKey(volumeName,
-                  bucketName);
-          OmBucketInfo omBucketInfo =
-                  metadataManager.getBucketTable().get(bucketKey);
-          prefixKeyInDB = omBucketInfo.getObjectID();
-        }
-        seekFileInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
-        seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
-
-        // Order of seek -> (1)Seek files in fileTable (2)Seek dirs in dirTable
-        // 1. Seek the given key in key table.
-        countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
-                prefixPath, prefixKeyInDB, startKey, countEntries, numEntries);
-        // 2. Seek the given key in dir table.
-        getDirectories(cacheDirMap, seekDirInDB, prefixPath, prefixKeyInDB,
-                startKey, countEntries, numEntries, volumeName, bucketName,
-                recursive);
-      } else {
-        /*
-         * startKey will be used in iterator seek and sets the beginning point
-         * for key traversal.
-         * keyName will be used as parentID where the user has requested to
-         * list the keys from.
-         *
-         * When recursive flag=false, parentID won't change between two pages.
-         * For example: OM has a namespace like,
-         *    /a/1...1M files and /a/b/1...1M files.
-         *    /a/1...1M directories and /a/b/1...1M directories.
-         * Listing "/a", will always have the parentID as "a" irrespective of
-         * the startKey value.
-         */
-
-        // Check startKey is an immediate child of keyName. For example,
-        // keyName=/a/ and expected startKey=/a/b. startKey can't be /xyz/b.
-        if (StringUtils.isNotBlank(keyName) &&
-                !OzoneFSUtils.isImmediateChild(keyName, startKey)) {
-          if (LOG.isDebugEnabled()) {
-            LOG.debug("StartKey {} is not an immediate child of keyName {}. " +
-                    "Returns empty list", startKey, keyName);
-          }
-          return Collections.emptyList();
-        }
 
-        // assign startKeyPath if prefixPath is empty string.
-        if (StringUtils.isBlank(prefixPath)) {
-          prefixPath = OzoneFSUtils.getParentDir(startKey);
+    if (Strings.isNullOrEmpty(startKey)) {
+      OzoneFileStatus fileStatus = getFileStatus(args, clientAddress);
+      if (fileStatus.isFile()) {
+        return Collections.singletonList(fileStatus);
+      }
+
+      // Not required to search in DeletedTable because all the deleted
+      // keys will be marked directly in dirTable or in keyTable by
+      // breaking the pointer to its sub-dirs and sub-files. So, there is no
+      // issue of inconsistency.
+
+      /*
+       * keyName is a directory.
+       * Say, "/a" is the dir name and its objectID is 1024, then seek
+       * will be doing with "1024/" to get all immediate descendants.
+       */
+      if (fileStatus.getKeyInfo() != null) {
+        prefixKeyInDB = fileStatus.getKeyInfo().getObjectID();
+      } else {
+        // list root directory.
+        String bucketKey = metadataManager.getBucketKey(volumeName, bucketName);
+        OmBucketInfo omBucketInfo =
+            metadataManager.getBucketTable().get(bucketKey);
+        prefixKeyInDB = omBucketInfo.getObjectID();
+      }
+      seekFileInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
+      seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
+
+      // Order of seek ->
+      // (1)Seek files in fileTable
+      // (2)Seek dirs in dirTable
+
+      Set<String> deletedKeySet = new TreeSet<>();
+      TreeMap<String, OzoneFileStatus> tempCacheDirMap = new TreeMap<>();
+
+      // Get files, dirs and marked for delete entries from cache.
+      countEntries = getFilesAndDirsFromCache(volumeName, bucketName,
+          cacheFileMap, tempCacheDirMap, deletedKeySet, prefixKeyInDB,
+          seekFileInDB, seekDirInDB, prefixPath, startKey, countEntries,
+          numEntries);
+
+      countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
+          prefixPath, prefixKeyInDB, countEntries, numEntries, deletedKeySet);
+
+      // Now first add all entries in the dir cache, if numEntries required
+      // is less than countEntries.
+      for (Map.Entry<String, OzoneFileStatus> dirEntry :
+          tempCacheDirMap.entrySet()) {
+        if (countEntries < numEntries) {
+          cacheDirMap.put(dirEntry.getKey(), dirEntry.getValue());
+          countEntries++;
         }
+      }
 
-        OzoneFileStatus fileStatusInfo = getOzoneFileStatusFSO(volumeName,
-                bucketName, startKey, false, null,
-                args.getLatestVersionLocation(), true);
+      // 2. Seek the given key in dir table.
+      if (countEntries < numEntries) {
+        getDirectories(cacheDirMap, seekDirInDB, prefixPath,
+            prefixKeyInDB, countEntries, numEntries, recursive,
+            volumeName, bucketName, deletedKeySet);
+      }
 
-        if (fileStatusInfo != null) {
-          prefixKeyInDB = fileStatusInfo.getKeyInfo().getParentObjectID();
-          if(fileStatusInfo.isDirectory()){
-            seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB,
-                    fileStatusInfo.getKeyInfo().getFileName());
+    } else {
+      /*
+       * startKey will be used in iterator seek and sets the beginning point
+       * for key traversal.
+       * keyName will be used as parentID where the user has requested to
+       * list the keys from.
+       *
+       * When recursive flag=false, parentID won't change between two pages.
+       * For example: OM has a namespace like,
+       *    /a/1...1M files and /a/b/1...1M files.
+       *    /a/1...1M directories and /a/b/1...1M directories.
+       * Listing "/a", will always have the parentID as "a" irrespective of
+       * the startKey value.
+       */
+
+      // Check startKey is an immediate child of keyName. For example,
+      // keyName=/a/ and expected startKey=/a/b. startKey can't be /xyz/b.
+      if (StringUtils.isNotBlank(keyName) &&
+          !OzoneFSUtils.isImmediateChild(keyName, startKey)) {
+        if (LOG.isDebugEnabled()) {
+          LOG.debug("StartKey {} is not an immediate child of keyName {}. " +
+              "Returns empty list", startKey, keyName);
+        }
+        return Collections.emptyList();
+      }
+
+      // assign startKeyPath if prefixPath is empty string.
+      if (StringUtils.isBlank(prefixPath)) {
+        prefixPath = OzoneFSUtils.getParentDir(startKey);
+      }
+
+      OzoneFileStatus fileStatusInfo = getOzoneFileStatusFSO(volumeName,
+          bucketName, startKey, false, null,
+          args.getLatestVersionLocation(), true);
+
+      if (fileStatusInfo != null) {
+        prefixKeyInDB = fileStatusInfo.getKeyInfo().getParentObjectID();
+        if(fileStatusInfo.isDirectory()){
+          seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB,
+              fileStatusInfo.getKeyInfo().getFileName());
+
+          // Order of seek -> (1) Seek dirs only in dirTable. In OM, always
+          // the order of search is, first seek into fileTable and then
+          // dirTable. So, its not required to search again in the fileTable.
+
+          // Seek the given key in dirTable.
+          metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,
+                bucketName);
+
+          TreeSet<String> deletedKeySet = new TreeSet<>();
+          TreeMap<String, OzoneFileStatus> tempCacheDirMap = new TreeMap<>();
+          try {
+            listStatusFindDirsInTableCache(tempCacheDirMap,
+                metadataManager.getDirectoryTable(),
+                prefixKeyInDB, seekDirInDB, prefixPath, startKey, volumeName,
+                bucketName, countEntries, numEntries, deletedKeySet);
+          } finally {
+            metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName,
+                bucketName);
+          }
 
-            // Order of seek -> (1) Seek dirs only in dirTable. In OM, always
-            // the order of search is, first seek into fileTable and then
-            // dirTable. So, its not required to search again in the fileTable.
+          // Now first add all entries in the dir cache, if numEntries required

Review comment:
       Done

##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -2318,110 +2319,111 @@ private void listStatusFindKeyInTableCache(
       startKey = OzoneFSUtils.addTrailingSlashIfNeeded(keyName);
     }
 
+    // Note: eliminating the case where startCacheKey could end with '//'
+    String keyArgs = OzoneFSUtils.addTrailingSlashIfNeeded(
+        metadataManager.getOzoneKey(volumeName, bucketName, keyName));
+
     metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,
         bucketName);
+
+    Table keyTable = metadataManager.getKeyTable();
     try {
-      Table keyTable = metadataManager.getKeyTable();
-      Iterator<Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>>>
+      Iterator< Map.Entry< CacheKey< String >, CacheValue< OmKeyInfo > > >
           cacheIter = keyTable.cacheIterator();
       String startCacheKey = OZONE_URI_DELIMITER + volumeName +
           OZONE_URI_DELIMITER + bucketName + OZONE_URI_DELIMITER +
           ((startKey.equals(OZONE_URI_DELIMITER)) ? "" : startKey);
-      // Note: eliminating the case where startCacheKey could end with '//'
-      String keyArgs = OzoneFSUtils.addTrailingSlashIfNeeded(
-          metadataManager.getOzoneKey(volumeName, bucketName, keyName));
 
       // First, find key in TableCache
       listStatusFindKeyInTableCache(cacheIter, keyArgs, startCacheKey,
           recursive, cacheKeyMap, deletedKeySet);
-      // Then, find key in DB
-      String seekKeyInDb =
-          metadataManager.getOzoneKey(volumeName, bucketName, startKey);
-      TableIterator<String, ? extends Table.KeyValue<String, OmKeyInfo>>
-          iterator = keyTable.iterator();
-      iterator.seek(seekKeyInDb);
-      int countEntries = 0;
-      if (iterator.hasNext()) {
-        if (iterator.key().equals(keyArgs)) {
-          // Skip the key itself, since we are listing inside the directory
-          iterator.next();
-        }
-        // Iterate through seek results
-        while (iterator.hasNext() && numEntries - countEntries > 0) {
-          String entryInDb = iterator.key();
-          OmKeyInfo omKeyInfo = iterator.value().getValue();
-          if (entryInDb.startsWith(keyArgs)) {
-            String entryKeyName = omKeyInfo.getKeyName();
-            if (recursive) {
-              // for recursive list all the entries
+    } finally {
+      metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName,
+          bucketName);
+    }
+
+    // Then, find key in DB
+    String seekKeyInDb =
+        metadataManager.getOzoneKey(volumeName, bucketName, startKey);
+    TableIterator<String, ? extends Table.KeyValue<String, OmKeyInfo>>
+        iterator = keyTable.iterator();

Review comment:
       moved inside lock




-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

[GitHub] [ozone] rakeshadr commented on a change in pull request #2369: HDDS-5349. Avoid usage of locks in listStatus.

[GitBox <gi...@apache.org>]

rakeshadr commented on a change in pull request #2369:
URL: https://github.com/apache/ozone/pull/2369#discussion_r663713965



##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -2318,110 +2319,111 @@ private void listStatusFindKeyInTableCache(
       startKey = OzoneFSUtils.addTrailingSlashIfNeeded(keyName);
     }
 
+    // Note: eliminating the case where startCacheKey could end with '//'
+    String keyArgs = OzoneFSUtils.addTrailingSlashIfNeeded(
+        metadataManager.getOzoneKey(volumeName, bucketName, keyName));
+
     metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,
         bucketName);
+
+    Table keyTable = metadataManager.getKeyTable();
     try {
-      Table keyTable = metadataManager.getKeyTable();
-      Iterator<Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>>>
+      Iterator< Map.Entry< CacheKey< String >, CacheValue< OmKeyInfo > > >
           cacheIter = keyTable.cacheIterator();
       String startCacheKey = OZONE_URI_DELIMITER + volumeName +
           OZONE_URI_DELIMITER + bucketName + OZONE_URI_DELIMITER +
           ((startKey.equals(OZONE_URI_DELIMITER)) ? "" : startKey);
-      // Note: eliminating the case where startCacheKey could end with '//'
-      String keyArgs = OzoneFSUtils.addTrailingSlashIfNeeded(
-          metadataManager.getOzoneKey(volumeName, bucketName, keyName));
 
       // First, find key in TableCache
       listStatusFindKeyInTableCache(cacheIter, keyArgs, startCacheKey,
           recursive, cacheKeyMap, deletedKeySet);
-      // Then, find key in DB
-      String seekKeyInDb =
-          metadataManager.getOzoneKey(volumeName, bucketName, startKey);
-      TableIterator<String, ? extends Table.KeyValue<String, OmKeyInfo>>
-          iterator = keyTable.iterator();
-      iterator.seek(seekKeyInDb);
-      int countEntries = 0;
-      if (iterator.hasNext()) {
-        if (iterator.key().equals(keyArgs)) {
-          // Skip the key itself, since we are listing inside the directory
-          iterator.next();
-        }
-        // Iterate through seek results
-        while (iterator.hasNext() && numEntries - countEntries > 0) {
-          String entryInDb = iterator.key();
-          OmKeyInfo omKeyInfo = iterator.value().getValue();
-          if (entryInDb.startsWith(keyArgs)) {
-            String entryKeyName = omKeyInfo.getKeyName();
-            if (recursive) {
-              // for recursive list all the entries
+    } finally {
+      metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName,
+          bucketName);
+    }
+
+    // Then, find key in DB
+    String seekKeyInDb =
+        metadataManager.getOzoneKey(volumeName, bucketName, startKey);
+    TableIterator<String, ? extends Table.KeyValue<String, OmKeyInfo>>
+        iterator = keyTable.iterator();

Review comment:
       `keyTable.iterator()` is kept outside bucket lock, is that intentional. If yes, can you please add java comments to make it clear to others. 

##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -2483,159 +2484,276 @@ private void listStatusFindKeyInTableCache(
     int countEntries = 0;
 
     // TODO: recursive flag=true will be handled in HDDS-4360 jira.
-    metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,
-            bucketName);
-    try {
-      if (Strings.isNullOrEmpty(startKey)) {
-        OzoneFileStatus fileStatus = getFileStatus(args, clientAddress);
-        if (fileStatus.isFile()) {
-          return Collections.singletonList(fileStatus);
-        }
 
-        // Not required to search in DeletedTable because all the deleted
-        // keys will be marked directly in dirTable or in keyTable by
-        // breaking the pointer to its sub-dirs and sub-files. So, there is no
-        // issue of inconsistency.
-
-        /*
-         * keyName is a directory.
-         * Say, "/a" is the dir name and its objectID is 1024, then seek
-         * will be doing with "1024/" to get all immediate descendants.
-         */
-        if (fileStatus.getKeyInfo() != null) {
-          prefixKeyInDB = fileStatus.getKeyInfo().getObjectID();
-        } else {
-          // list root directory.
-          String bucketKey = metadataManager.getBucketKey(volumeName,
-                  bucketName);
-          OmBucketInfo omBucketInfo =
-                  metadataManager.getBucketTable().get(bucketKey);
-          prefixKeyInDB = omBucketInfo.getObjectID();
-        }
-        seekFileInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
-        seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
-
-        // Order of seek -> (1)Seek files in fileTable (2)Seek dirs in dirTable
-        // 1. Seek the given key in key table.
-        countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
-                prefixPath, prefixKeyInDB, startKey, countEntries, numEntries);
-        // 2. Seek the given key in dir table.
-        getDirectories(cacheDirMap, seekDirInDB, prefixPath, prefixKeyInDB,
-                startKey, countEntries, numEntries, volumeName, bucketName,
-                recursive);
-      } else {
-        /*
-         * startKey will be used in iterator seek and sets the beginning point
-         * for key traversal.
-         * keyName will be used as parentID where the user has requested to
-         * list the keys from.
-         *
-         * When recursive flag=false, parentID won't change between two pages.
-         * For example: OM has a namespace like,
-         *    /a/1...1M files and /a/b/1...1M files.
-         *    /a/1...1M directories and /a/b/1...1M directories.
-         * Listing "/a", will always have the parentID as "a" irrespective of
-         * the startKey value.
-         */
-
-        // Check startKey is an immediate child of keyName. For example,
-        // keyName=/a/ and expected startKey=/a/b. startKey can't be /xyz/b.
-        if (StringUtils.isNotBlank(keyName) &&
-                !OzoneFSUtils.isImmediateChild(keyName, startKey)) {
-          if (LOG.isDebugEnabled()) {
-            LOG.debug("StartKey {} is not an immediate child of keyName {}. " +
-                    "Returns empty list", startKey, keyName);
-          }
-          return Collections.emptyList();
-        }
 
-        // assign startKeyPath if prefixPath is empty string.
-        if (StringUtils.isBlank(prefixPath)) {
-          prefixPath = OzoneFSUtils.getParentDir(startKey);
+    if (Strings.isNullOrEmpty(startKey)) {
+      OzoneFileStatus fileStatus = getFileStatus(args, clientAddress);
+      if (fileStatus.isFile()) {
+        return Collections.singletonList(fileStatus);
+      }
+
+      // Not required to search in DeletedTable because all the deleted
+      // keys will be marked directly in dirTable or in keyTable by
+      // breaking the pointer to its sub-dirs and sub-files. So, there is no
+      // issue of inconsistency.
+
+      /*
+       * keyName is a directory.
+       * Say, "/a" is the dir name and its objectID is 1024, then seek
+       * will be doing with "1024/" to get all immediate descendants.
+       */
+      if (fileStatus.getKeyInfo() != null) {
+        prefixKeyInDB = fileStatus.getKeyInfo().getObjectID();
+      } else {
+        // list root directory.
+        String bucketKey = metadataManager.getBucketKey(volumeName, bucketName);
+        OmBucketInfo omBucketInfo =
+            metadataManager.getBucketTable().get(bucketKey);
+        prefixKeyInDB = omBucketInfo.getObjectID();
+      }
+      seekFileInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
+      seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
+
+      // Order of seek ->
+      // (1)Seek files in fileTable
+      // (2)Seek dirs in dirTable
+
+      Set<String> deletedKeySet = new TreeSet<>();
+      TreeMap<String, OzoneFileStatus> tempCacheDirMap = new TreeMap<>();
+
+      // Get files, dirs and marked for delete entries from cache.
+      countEntries = getFilesAndDirsFromCache(volumeName, bucketName,
+          cacheFileMap, tempCacheDirMap, deletedKeySet, prefixKeyInDB,
+          seekFileInDB, seekDirInDB, prefixPath, startKey, countEntries,
+          numEntries);
+
+      countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
+          prefixPath, prefixKeyInDB, countEntries, numEntries, deletedKeySet);
+
+      // Now first add all entries in the dir cache, if numEntries required
+      // is less than countEntries.
+      for (Map.Entry<String, OzoneFileStatus> dirEntry :
+          tempCacheDirMap.entrySet()) {
+        if (countEntries < numEntries) {
+          cacheDirMap.put(dirEntry.getKey(), dirEntry.getValue());
+          countEntries++;
         }
+      }
 
-        OzoneFileStatus fileStatusInfo = getOzoneFileStatusFSO(volumeName,
-                bucketName, startKey, false, null,
-                args.getLatestVersionLocation(), true);
+      // 2. Seek the given key in dir table.
+      if (countEntries < numEntries) {
+        getDirectories(cacheDirMap, seekDirInDB, prefixPath,
+            prefixKeyInDB, countEntries, numEntries, recursive,
+            volumeName, bucketName, deletedKeySet);
+      }
 
-        if (fileStatusInfo != null) {
-          prefixKeyInDB = fileStatusInfo.getKeyInfo().getParentObjectID();
-          if(fileStatusInfo.isDirectory()){
-            seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB,
-                    fileStatusInfo.getKeyInfo().getFileName());
+    } else {
+      /*
+       * startKey will be used in iterator seek and sets the beginning point
+       * for key traversal.
+       * keyName will be used as parentID where the user has requested to
+       * list the keys from.
+       *
+       * When recursive flag=false, parentID won't change between two pages.
+       * For example: OM has a namespace like,
+       *    /a/1...1M files and /a/b/1...1M files.
+       *    /a/1...1M directories and /a/b/1...1M directories.
+       * Listing "/a", will always have the parentID as "a" irrespective of
+       * the startKey value.
+       */
+
+      // Check startKey is an immediate child of keyName. For example,
+      // keyName=/a/ and expected startKey=/a/b. startKey can't be /xyz/b.
+      if (StringUtils.isNotBlank(keyName) &&
+          !OzoneFSUtils.isImmediateChild(keyName, startKey)) {
+        if (LOG.isDebugEnabled()) {
+          LOG.debug("StartKey {} is not an immediate child of keyName {}. " +
+              "Returns empty list", startKey, keyName);
+        }
+        return Collections.emptyList();
+      }
+
+      // assign startKeyPath if prefixPath is empty string.
+      if (StringUtils.isBlank(prefixPath)) {
+        prefixPath = OzoneFSUtils.getParentDir(startKey);
+      }
+
+      OzoneFileStatus fileStatusInfo = getOzoneFileStatusFSO(volumeName,
+          bucketName, startKey, false, null,
+          args.getLatestVersionLocation(), true);
+
+      if (fileStatusInfo != null) {
+        prefixKeyInDB = fileStatusInfo.getKeyInfo().getParentObjectID();
+        if(fileStatusInfo.isDirectory()){
+          seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB,
+              fileStatusInfo.getKeyInfo().getFileName());
+
+          // Order of seek -> (1) Seek dirs only in dirTable. In OM, always
+          // the order of search is, first seek into fileTable and then
+          // dirTable. So, its not required to search again in the fileTable.
+
+          // Seek the given key in dirTable.
+          metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,
+                bucketName);
+
+          TreeSet<String> deletedKeySet = new TreeSet<>();
+          TreeMap<String, OzoneFileStatus> tempCacheDirMap = new TreeMap<>();
+          try {
+            listStatusFindDirsInTableCache(tempCacheDirMap,
+                metadataManager.getDirectoryTable(),
+                prefixKeyInDB, seekDirInDB, prefixPath, startKey, volumeName,
+                bucketName, countEntries, numEntries, deletedKeySet);
+          } finally {
+            metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName,
+                bucketName);
+          }
 
-            // Order of seek -> (1) Seek dirs only in dirTable. In OM, always
-            // the order of search is, first seek into fileTable and then
-            // dirTable. So, its not required to search again in the fileTable.
+          // Now first add all entries in the dir cache, if numEntries required

Review comment:
       Below logic is duplicated in both if and else blocks. Line-2611 and line-2646. Can we move this outside the if-else block to avoid duplicate?
   
   ```
             for (Map.Entry<String, OzoneFileStatus> dirEntry :
                 tempCacheDirMap.entrySet()) {
               if (countEntries < numEntries) {
                 cacheDirMap.put(dirEntry.getKey(), dirEntry.getValue());
                 countEntries++;
               }
             }
   
             if (countEntries < numEntries) {
               getDirectories(cacheDirMap, seekDirInDB, prefixPath,
                   prefixKeyInDB, countEntries, numEntries, recursive,
                   volumeName, bucketName, deletedKeySet);
             }
   
   ```

##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -2483,159 +2484,276 @@ private void listStatusFindKeyInTableCache(
     int countEntries = 0;
 
     // TODO: recursive flag=true will be handled in HDDS-4360 jira.
-    metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,
-            bucketName);
-    try {
-      if (Strings.isNullOrEmpty(startKey)) {
-        OzoneFileStatus fileStatus = getFileStatus(args, clientAddress);
-        if (fileStatus.isFile()) {
-          return Collections.singletonList(fileStatus);
-        }
 
-        // Not required to search in DeletedTable because all the deleted
-        // keys will be marked directly in dirTable or in keyTable by
-        // breaking the pointer to its sub-dirs and sub-files. So, there is no
-        // issue of inconsistency.
-
-        /*
-         * keyName is a directory.
-         * Say, "/a" is the dir name and its objectID is 1024, then seek
-         * will be doing with "1024/" to get all immediate descendants.
-         */
-        if (fileStatus.getKeyInfo() != null) {
-          prefixKeyInDB = fileStatus.getKeyInfo().getObjectID();
-        } else {
-          // list root directory.
-          String bucketKey = metadataManager.getBucketKey(volumeName,
-                  bucketName);
-          OmBucketInfo omBucketInfo =
-                  metadataManager.getBucketTable().get(bucketKey);
-          prefixKeyInDB = omBucketInfo.getObjectID();
-        }
-        seekFileInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
-        seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
-
-        // Order of seek -> (1)Seek files in fileTable (2)Seek dirs in dirTable
-        // 1. Seek the given key in key table.
-        countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
-                prefixPath, prefixKeyInDB, startKey, countEntries, numEntries);
-        // 2. Seek the given key in dir table.
-        getDirectories(cacheDirMap, seekDirInDB, prefixPath, prefixKeyInDB,
-                startKey, countEntries, numEntries, volumeName, bucketName,
-                recursive);
-      } else {
-        /*
-         * startKey will be used in iterator seek and sets the beginning point
-         * for key traversal.
-         * keyName will be used as parentID where the user has requested to
-         * list the keys from.
-         *
-         * When recursive flag=false, parentID won't change between two pages.
-         * For example: OM has a namespace like,
-         *    /a/1...1M files and /a/b/1...1M files.
-         *    /a/1...1M directories and /a/b/1...1M directories.
-         * Listing "/a", will always have the parentID as "a" irrespective of
-         * the startKey value.
-         */
-
-        // Check startKey is an immediate child of keyName. For example,
-        // keyName=/a/ and expected startKey=/a/b. startKey can't be /xyz/b.
-        if (StringUtils.isNotBlank(keyName) &&
-                !OzoneFSUtils.isImmediateChild(keyName, startKey)) {
-          if (LOG.isDebugEnabled()) {
-            LOG.debug("StartKey {} is not an immediate child of keyName {}. " +
-                    "Returns empty list", startKey, keyName);
-          }
-          return Collections.emptyList();
-        }
 
-        // assign startKeyPath if prefixPath is empty string.
-        if (StringUtils.isBlank(prefixPath)) {
-          prefixPath = OzoneFSUtils.getParentDir(startKey);
+    if (Strings.isNullOrEmpty(startKey)) {
+      OzoneFileStatus fileStatus = getFileStatus(args, clientAddress);
+      if (fileStatus.isFile()) {
+        return Collections.singletonList(fileStatus);
+      }
+
+      // Not required to search in DeletedTable because all the deleted
+      // keys will be marked directly in dirTable or in keyTable by
+      // breaking the pointer to its sub-dirs and sub-files. So, there is no
+      // issue of inconsistency.
+
+      /*
+       * keyName is a directory.
+       * Say, "/a" is the dir name and its objectID is 1024, then seek
+       * will be doing with "1024/" to get all immediate descendants.
+       */
+      if (fileStatus.getKeyInfo() != null) {
+        prefixKeyInDB = fileStatus.getKeyInfo().getObjectID();
+      } else {
+        // list root directory.
+        String bucketKey = metadataManager.getBucketKey(volumeName, bucketName);
+        OmBucketInfo omBucketInfo =
+            metadataManager.getBucketTable().get(bucketKey);
+        prefixKeyInDB = omBucketInfo.getObjectID();
+      }
+      seekFileInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
+      seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
+
+      // Order of seek ->
+      // (1)Seek files in fileTable
+      // (2)Seek dirs in dirTable
+
+      Set<String> deletedKeySet = new TreeSet<>();
+      TreeMap<String, OzoneFileStatus> tempCacheDirMap = new TreeMap<>();
+
+      // Get files, dirs and marked for delete entries from cache.
+      countEntries = getFilesAndDirsFromCache(volumeName, bucketName,
+          cacheFileMap, tempCacheDirMap, deletedKeySet, prefixKeyInDB,
+          seekFileInDB, seekDirInDB, prefixPath, startKey, countEntries,
+          numEntries);
+
+      countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
+          prefixPath, prefixKeyInDB, countEntries, numEntries, deletedKeySet);
+
+      // Now first add all entries in the dir cache, if numEntries required
+      // is less than countEntries.
+      for (Map.Entry<String, OzoneFileStatus> dirEntry :
+          tempCacheDirMap.entrySet()) {
+        if (countEntries < numEntries) {
+          cacheDirMap.put(dirEntry.getKey(), dirEntry.getValue());
+          countEntries++;
         }
+      }
 
-        OzoneFileStatus fileStatusInfo = getOzoneFileStatusFSO(volumeName,
-                bucketName, startKey, false, null,
-                args.getLatestVersionLocation(), true);
+      // 2. Seek the given key in dir table.
+      if (countEntries < numEntries) {
+        getDirectories(cacheDirMap, seekDirInDB, prefixPath,
+            prefixKeyInDB, countEntries, numEntries, recursive,
+            volumeName, bucketName, deletedKeySet);
+      }
 
-        if (fileStatusInfo != null) {
-          prefixKeyInDB = fileStatusInfo.getKeyInfo().getParentObjectID();
-          if(fileStatusInfo.isDirectory()){
-            seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB,
-                    fileStatusInfo.getKeyInfo().getFileName());
+    } else {
+      /*
+       * startKey will be used in iterator seek and sets the beginning point
+       * for key traversal.
+       * keyName will be used as parentID where the user has requested to
+       * list the keys from.
+       *
+       * When recursive flag=false, parentID won't change between two pages.
+       * For example: OM has a namespace like,
+       *    /a/1...1M files and /a/b/1...1M files.
+       *    /a/1...1M directories and /a/b/1...1M directories.
+       * Listing "/a", will always have the parentID as "a" irrespective of
+       * the startKey value.
+       */
+
+      // Check startKey is an immediate child of keyName. For example,
+      // keyName=/a/ and expected startKey=/a/b. startKey can't be /xyz/b.
+      if (StringUtils.isNotBlank(keyName) &&
+          !OzoneFSUtils.isImmediateChild(keyName, startKey)) {
+        if (LOG.isDebugEnabled()) {
+          LOG.debug("StartKey {} is not an immediate child of keyName {}. " +
+              "Returns empty list", startKey, keyName);
+        }
+        return Collections.emptyList();
+      }
+
+      // assign startKeyPath if prefixPath is empty string.
+      if (StringUtils.isBlank(prefixPath)) {
+        prefixPath = OzoneFSUtils.getParentDir(startKey);
+      }
+
+      OzoneFileStatus fileStatusInfo = getOzoneFileStatusFSO(volumeName,
+          bucketName, startKey, false, null,
+          args.getLatestVersionLocation(), true);
+
+      if (fileStatusInfo != null) {
+        prefixKeyInDB = fileStatusInfo.getKeyInfo().getParentObjectID();
+        if(fileStatusInfo.isDirectory()){
+          seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB,
+              fileStatusInfo.getKeyInfo().getFileName());
+
+          // Order of seek -> (1) Seek dirs only in dirTable. In OM, always
+          // the order of search is, first seek into fileTable and then
+          // dirTable. So, its not required to search again in the fileTable.
+
+          // Seek the given key in dirTable.
+          metadataManager.getLock().acquireReadLock(BUCKET_LOCK, volumeName,
+                bucketName);
+
+          TreeSet<String> deletedKeySet = new TreeSet<>();
+          TreeMap<String, OzoneFileStatus> tempCacheDirMap = new TreeMap<>();
+          try {
+            listStatusFindDirsInTableCache(tempCacheDirMap,
+                metadataManager.getDirectoryTable(),
+                prefixKeyInDB, seekDirInDB, prefixPath, startKey, volumeName,
+                bucketName, countEntries, numEntries, deletedKeySet);
+          } finally {
+            metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName,
+                bucketName);
+          }
 
-            // Order of seek -> (1) Seek dirs only in dirTable. In OM, always
-            // the order of search is, first seek into fileTable and then
-            // dirTable. So, its not required to search again in the fileTable.
+          // Now first add all entries in the dir cache, if numEntries required
+          // is less than countEntries.
+          for (Map.Entry<String, OzoneFileStatus> dirEntry :
+              tempCacheDirMap.entrySet()) {
+            if (countEntries < numEntries) {
+              cacheDirMap.put(dirEntry.getKey(), dirEntry.getValue());
+              countEntries++;
+            }
+          }
 
-            // Seek the given key in dirTable.
-            getDirectories(cacheDirMap, seekDirInDB, prefixPath,
-                    prefixKeyInDB, startKey, countEntries, numEntries,
-                    volumeName, bucketName, recursive);
-          } else {
-            seekFileInDB = metadataManager.getOzonePathKey(prefixKeyInDB,
-                    fileStatusInfo.getKeyInfo().getFileName());
-            // begins from the first sub-dir under the parent dir
-            seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
-
-            // 1. Seek the given key in key table.
-            countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
-                    prefixPath, prefixKeyInDB, startKey, countEntries,
-                    numEntries);
-            // 2. Seek the given key in dir table.
+          if (countEntries < numEntries) {
             getDirectories(cacheDirMap, seekDirInDB, prefixPath,
-                    prefixKeyInDB, startKey, countEntries, numEntries,
-                    volumeName, bucketName, recursive);
+                prefixKeyInDB, countEntries, numEntries, recursive,
+                volumeName, bucketName, deletedKeySet);
           }
         } else {
-          // TODO: HDDS-4364: startKey can be a non-existed key
-          if (LOG.isDebugEnabled()) {
-            LOG.debug("StartKey {} is a non-existed key and returning empty " +
-                    "list", startKey);
+          seekFileInDB = metadataManager.getOzonePathKey(prefixKeyInDB,
+              fileStatusInfo.getKeyInfo().getFileName());
+          // begins from the first sub-dir under the parent dir
+          seekDirInDB = metadataManager.getOzonePathKey(prefixKeyInDB, "");
+
+          TreeSet<String> deletedKeySet = new TreeSet<>();
+          TreeMap<String, OzoneFileStatus> tempCacheDirMap = new TreeMap<>();
+
+          // Get files, dirs and marked for delete entries from cache.
+          countEntries = getFilesAndDirsFromCache(volumeName, bucketName,
+              cacheFileMap, tempCacheDirMap, deletedKeySet, prefixKeyInDB,
+              seekFileInDB, seekDirInDB, prefixPath, startKey, countEntries,
+              numEntries);
+
+          // 1. Seek the given key in key table.
+          countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
+              prefixPath, prefixKeyInDB, countEntries, numEntries,
+              deletedKeySet);
+
+          // Now first add all entries in the dir cache, if numEntries required
+          // is less than countEntries.
+          for (Map.Entry<String, OzoneFileStatus> dirEntry :
+              tempCacheDirMap.entrySet()) {
+            if (countEntries < numEntries) {
+              cacheDirMap.put(dirEntry.getKey(), dirEntry.getValue());
+              countEntries++;
+            }
+          }
+
+          // 2. Seek the given key in dir table.
+          if (countEntries < numEntries) {
+            getDirectories(cacheDirMap, seekDirInDB, prefixPath,
+                prefixKeyInDB, countEntries, numEntries, recursive,
+                volumeName, bucketName, deletedKeySet);
           }
-          return Collections.emptyList();
         }
+      } else {
+        // TODO: HDDS-4364: startKey can be a non-existed key
+        if (LOG.isDebugEnabled()) {
+          LOG.debug("StartKey {} is a non-existed key and returning empty " +
+                    "list", startKey);
+        }
+        return Collections.emptyList();
       }
-    } finally {
-      metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName,
-              bucketName);
     }
 
+    return buildFinalStatusList(cacheFileMap, cacheDirMap, args, clientAddress);
+
+  }
+
+  /**
+   * Build final OzoneFileStatus list to be returned to client.
+   * @throws IOException
+   */
+  private List<OzoneFileStatus> buildFinalStatusList(
+      Map<String, OzoneFileStatus> cacheFileMap,
+      Map<String, OzoneFileStatus> cacheDirMap, OmKeyArgs omKeyArgs,
+      String clientAddress)
+      throws IOException {
     List<OmKeyInfo> keyInfoList = new ArrayList<>();
+    List<OzoneFileStatus> fileStatusFinalList = new ArrayList<>();
+
     for (OzoneFileStatus fileStatus : cacheFileMap.values()) {
       fileStatusFinalList.add(fileStatus);
       keyInfoList.add(fileStatus.getKeyInfo());
     }
+
     for (OzoneFileStatus fileStatus : cacheDirMap.values()) {
       fileStatusFinalList.add(fileStatus);
     }
-    if (args.getLatestVersionLocation()) {
+
+    if (omKeyArgs.getLatestVersionLocation()) {
       slimLocationVersion(keyInfoList.toArray(new OmKeyInfo[0]));
     }
     // refreshPipeline flag check has been removed as part of
     // https://issues.apache.org/jira/browse/HDDS-3658.
     // Please refer this jira for more details.
     refreshPipeline(keyInfoList);
-    if (args.getSortDatanodes()) {
+
+    if (omKeyArgs.getSortDatanodes()) {
       sortDatanodes(clientAddress, keyInfoList.toArray(new OmKeyInfo[0]));
     }
+
     return fileStatusFinalList;
   }
 
+  /***
+   * Build files, directories and marked for deleted entries from dir/file
+   * cache.
+   */
+  @SuppressWarnings("parameternumber")
+  private int getFilesAndDirsFromCache(String volumeName, String bucketName,

Review comment:
       Can we name the function to reflect bucket_lock behavior, something like, `getFilesAndDirsFromCacheWithBucketLock`




-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

[GitHub] [ozone] rakeshadr commented on a change in pull request #2369: HDDS-5349. Avoid usage of locks in listStatus.

[GitBox <gi...@apache.org>]

rakeshadr commented on a change in pull request #2369:
URL: https://github.com/apache/ozone/pull/2369#discussion_r693673458



##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -2688,15 +2771,11 @@ protected int getDirectories(
   private int getFilesFromDirectory(
       TreeMap<String, OzoneFileStatus> cacheKeyMap,
       String seekKeyInDB, String prefixKeyPath, long prefixKeyInDB,
-      String startKey, int countEntries, long numEntries) throws IOException {
-
-    // A set to keep track of keys deleted in cache but not flushed to DB.
-    Set<String> deletedKeySet = new TreeSet<>();
+      int countEntries, long numEntries, Set<String> deletedKeySet)
+      throws IOException {
 
     Table<String, OmKeyInfo> keyTable = metadataManager.getKeyTable();
-    countEntries = listStatusFindFilesInTableCache(cacheKeyMap, keyTable,
-            prefixKeyInDB, seekKeyInDB, prefixKeyPath, startKey,
-            countEntries, numEntries, deletedKeySet);
+
     TableIterator<String, ? extends Table.KeyValue<String, OmKeyInfo>>
             iterator = keyTable.iterator();
     iterator.seek(seekKeyInDB);

Review comment:
       In SIMPLE layout code path, `iterator = keyTable.iterator();` is inside BUCKET_LOCK.  How about moving the BUCKET_LOCK in PREFIX layout something like,
   
   ```
   // First under lock obtain both entries from dir/file cache and generate
         // entries marked for delete.
         try {
           iterator = keyTable.iterator();
           countEntries = getFilesAndDirsFromCacheWithBucketLock(volumeName,
               bucketName, cacheFileMap, tempCacheDirMap, deletedKeySet,
               prefixKeyInDB, seekFileInDB, seekDirInDB, prefixPath, startKey,
               countEntries, numEntries);
         } finally {
           metadataManager.getLock().releaseReadLock(BUCKET_LOCK, volumeName,
               bucketName);
         }
         countEntries = getFilesFromDirectory(cacheFileMap, seekFileInDB,
             prefixPath, prefixKeyInDB, countEntries, numEntries, deletedKeySet,
             iterator);
   
   ```
   Reference: https://github.com/apache/ozone/pull/2369/files#diff-bde0dade7dd5ddda419499f4f999d25d40fcec1412e0ce809c36ffd1be473f22R2357
   




-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

[GitHub] [ozone] bharatviswa504 commented on a change in pull request #2369: HDDS-5349. Avoid usage of locks in listStatus.

[GitBox <gi...@apache.org>]

bharatviswa504 commented on a change in pull request #2369:
URL: https://github.com/apache/ozone/pull/2369#discussion_r722742216



##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -2688,15 +2771,11 @@ protected int getDirectories(
   private int getFilesFromDirectory(
       TreeMap<String, OzoneFileStatus> cacheKeyMap,
       String seekKeyInDB, String prefixKeyPath, long prefixKeyInDB,
-      String startKey, int countEntries, long numEntries) throws IOException {
-
-    // A set to keep track of keys deleted in cache but not flushed to DB.
-    Set<String> deletedKeySet = new TreeSet<>();
+      int countEntries, long numEntries, Set<String> deletedKeySet)
+      throws IOException {
 
     Table<String, OmKeyInfo> keyTable = metadataManager.getKeyTable();
-    countEntries = listStatusFindFilesInTableCache(cacheKeyMap, keyTable,
-            prefixKeyInDB, seekKeyInDB, prefixKeyPath, startKey,
-            countEntries, numEntries, deletedKeySet);
+
     TableIterator<String, ? extends Table.KeyValue<String, OmKeyInfo>>
             iterator = keyTable.iterator();
     iterator.seek(seekKeyInDB);

Review comment:
       @rakeshadr done




-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org