sodonnel commented on a change in pull request #3227:
URL: https://github.com/apache/ozone/pull/3227#discussion_r833391940



##########
File path: 
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OmMetadataManagerImpl.java
##########
@@ -708,43 +708,101 @@ public boolean isVolumeEmpty(String volume) throws 
IOException {
   public boolean isBucketEmpty(String volume, String bucket)
       throws IOException {
     String keyPrefix = getBucketKey(volume, bucket);
+    OmBucketInfo omBucketInfo = getBucketTable().get(keyPrefix);
+    BucketLayout bucketLayout = omBucketInfo.getBucketLayout();
 
-    // First check in key table cache.
-    Iterator<Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>>> iterator =
-        ((TypedTable< String, OmKeyInfo>) keyTable).cacheIterator();
-    while (iterator.hasNext()) {
-      Map.Entry< CacheKey<String>, CacheValue<OmKeyInfo>> entry =
-          iterator.next();
-      String key = entry.getKey().getCacheKey();
-      OmKeyInfo omKeyInfo = entry.getValue().getCacheValue();
-      // Making sure that entry is not for delete key request.
-      if (key.startsWith(keyPrefix) && omKeyInfo != null) {
-        return false;
-      }
-    }
-    try (TableIterator<String, ? extends KeyValue<String, OmKeyInfo>> keyIter =
-        keyTable.iterator()) {
-      KeyValue<String, OmKeyInfo> kv = keyIter.seek(keyPrefix);
-
-      if (kv != null) {
-        // Check the entry in db is not marked for delete. This can happen
-        // while entry is marked for delete, but it is not flushed to DB.
-        CacheValue<OmKeyInfo> cacheValue =
-            keyTable.getCacheValue(new CacheKey(kv.getKey()));
-        if (cacheValue != null) {
-          if (kv.getKey().startsWith(keyPrefix)
-              && cacheValue.getCacheValue() != null) {
-            return false; // we found at least one key with this vol/bucket
-            // prefix.
+    // FSO Bucket
+    if (bucketLayout.isFileSystemOptimized()) {
+      String bucketId = String.valueOf(omBucketInfo.getObjectID());
+      // We want to check if there are any directories or files
+      // that are present under this bucket.
+      // In buckets with FSO layout, the metadata entry for a
+      //  1. TOP-LEVEL DIRECTORY would be of the format <bucket ID>/dirName
+      //     inside the dirTable.
+      //  2. TOP-LEVEL FILE (a file directly placed under the bucket without
+      //     any sub paths) would be of the format <bucket ID>/fileName inside
+      //     the fileTable.
+
+      // Iterate over the keyTable and fileTable
+      for (Table table : new Table[]{dirTable, fileTable}) {
+        // First check in the fileTable and dirTable cache
+        Iterator<Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>>> iterator =
+            table.cacheIterator();
+        while (iterator.hasNext()) {
+          Map.Entry<CacheKey<String>, CacheValue<OmKeyInfo>> entry =
+              iterator.next();
+          String key = entry.getKey().getCacheKey();
+          OmKeyInfo omKeyInfo = entry.getValue().getCacheValue();
+          if (key.startsWith(bucketId) && omKeyInfo != null) {
+            return false;
           }
-        } else {
-          if (kv.getKey().startsWith(keyPrefix)) {
-            return false; // we found at least one key with this vol/bucket
-            // prefix.
+        }
+
+        // Check in the table
+        try (
+            TableIterator<String, ? extends KeyValue<String, OmKeyInfo>>
+                keyIter = table.iterator()) {
+          KeyValue<String, OmKeyInfo> kv = keyIter.seek(bucketId);
+
+          if (kv != null) {
+            // Check the entry in db is not marked for delete. This can happen
+            // while entry is marked for delete, but it is not flushed to DB.
+            CacheValue<OmKeyInfo> cacheValue =
+                table.getCacheValue(new CacheKey(kv.getKey()));
+            if (cacheValue != null) {
+              if (kv.getKey().startsWith(bucketId)
+                  && cacheValue.getCacheValue() != null) {
+                return false; // we found at least one file/dir under this
+                // bucket
+              }
+            } else {
+              if (kv.getKey().startsWith(bucketId)) {
+                return false; // we found at least one file/dir under this
+                // bucket.
+              }
+            }
           }
         }
       }
+    } else {

Review comment:
       The else branch seems to have a lot of duplication. Could we extract the 
bulk of the logic into a method and call the same method for both cases?
   
   Also, would it make sense to structure the code like:
   
   ```
   checkKeyTable
   if (FSO bucket) {
     checkDirTable
   }
   ```
   As it seems like the check in the key table is common to both?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]



---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to