JCLOUDS-1371: Optimize filesystem delimiter

populateBlobKeysInContainer will no longer recurse when the delimiter matches "/". This makes listing deep hierarchies with a delimiter faster. Note that the general LocalBlobStore handling is still required for the general cases. This requires removing a bogus test case. References gaul/s3proxy#473.
apache · Jan 24, 2023 · 62632c9 · 62632c9
1 parent e478dd5
commit 62632c9
Show file tree

Hide file tree

Showing 6 changed files with 18 additions and 36 deletions.
diff --git a/...src/main/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImpl.java b/...src/main/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImpl.java
@@ -342,7 +342,7 @@ public boolean blobExists(String container, String key) {
     * @throws IOException
     */
    @Override
-   public Iterable<String> getBlobKeysInsideContainer(String container, String prefix) throws IOException {
+   public Iterable<String> getBlobKeysInsideContainer(String container, String prefix, String delimiter) throws IOException {
       filesystemContainerNameValidator.validate(container);
       // check if container exists
       // TODO maybe an error is more appropriate
@@ -361,7 +361,7 @@ public Iterable<String> getBlobKeysInsideContainer(String container, String pref
          }
       }
 
-      populateBlobKeysInContainer(containerFile, blobNames, prefix, new Function<String, String>() {
+      populateBlobKeysInContainer(containerFile, blobNames, prefix, delimiter, new Function<String, String>() {
          @Override
          public String apply(String string) {
             return denormalize(string.substring(containerPathLength));
@@ -464,6 +464,7 @@ public Blob getBlob(final String container, final String key) {
                .contentType(contentType)
                .expires(expires)
                .tier(tier)
+               .type(isDirectory ? StorageType.FOLDER : StorageType.BLOB)
                .userMetadata(userMetadata.build());
          } else {
             builder.payload(byteSource)
@@ -770,7 +771,7 @@ public void deleteDirectory(String container, String directory) {
    public long countBlobs(String container, ListContainerOptions options) {
       // TODO: honor options
       try {
-         return Iterables.size(getBlobKeysInsideContainer(container, null));
+         return Iterables.size(getBlobKeysInsideContainer(container, null, null));
       } catch (IOException ioe) {
          throw Throwables.propagate(ioe);
       }
@@ -981,7 +982,7 @@ private File openFolder(String folderName) throws IOException {
    }
 
    private static void populateBlobKeysInContainer(File directory, Set<String> blobNames,
-         String prefix, Function<String, String> function) {
+         String prefix, String delimiter, Function<String, String> function) {
       File[] children = directory.listFiles();
       if (children == null) {
          return;
@@ -1001,7 +1002,11 @@ private static void populateBlobKeysInContainer(File directory, Set<String> blob
                continue;
             }
             blobNames.add(fullPath + File.separator); // TODO: undo if failures
-            populateBlobKeysInContainer(child, blobNames, prefix, function);
+            // Skip recursion if the delimiter tells us not to return children.
+            if (delimiter != null && delimiter.equals("/")) {
+               continue;
+            }
+            populateBlobKeysInContainer(child, blobNames, prefix, delimiter, function);
          }
       }
    }

diff --git a/apis/filesystem/src/test/java/org/jclouds/filesystem/FilesystemBlobStoreTest.java b/apis/filesystem/src/test/java/org/jclouds/filesystem/FilesystemBlobStoreTest.java
@@ -64,7 +64,6 @@
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 
-import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;
 import com.google.common.io.ByteSource;
 import com.google.common.io.Files;
@@ -527,20 +526,6 @@ public void testGetDirectoryBlob() throws IOException {
                 blobKey.substring(0, blobKey.length() - 1)));
     }
 
-    @Test(dataProvider = "ignoreOnMacOSX")
-    public void testListDirectoryBlobs() {
-        blobStore.createContainerInLocation(null, CONTAINER_NAME);
-        checkForContainerContent(CONTAINER_NAME, null);
-
-        Set<String> dirs = ImmutableSet.of(TestUtils.createRandomBlobKey("directory-", File.separator));
-        for (String d : dirs) {
-            blobStore.putBlob(CONTAINER_NAME, createDirBlob(d));
-            assertTrue(blobStore.blobExists(CONTAINER_NAME, d));
-        }
-
-        checkForContainerContent(CONTAINER_NAME, dirs);
-    }
-
     @Test(dataProvider = "ignoreOnMacOSX")
     public void testListDirectoryBlobsS3FS() {
         blobStore.createContainerInLocation(null, CONTAINER_NAME);

diff --git a/...test/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImplTest.java b/...test/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImplTest.java
@@ -427,7 +427,7 @@ public void testListDirectoryBlob() throws IOException {
       Blob blob = storageStrategy.newBlob(blobKey);
       storageStrategy.putBlob(CONTAINER_NAME, blob);
 
-      Iterable<String> keys = storageStrategy.getBlobKeysInsideContainer(CONTAINER_NAME, null);
+      Iterable<String> keys = storageStrategy.getBlobKeysInsideContainer(CONTAINER_NAME, null, null);
       Iterator<String> iter = keys.iterator();
       assertTrue(iter.hasNext());
       assertEquals(iter.next(), blobKey);
@@ -598,7 +598,7 @@ public void testGetBlobKeysInsideContainer() throws IOException {
       Iterable<String> resultList;
 
       // no container
-      resultList = storageStrategy.getBlobKeysInsideContainer(CONTAINER_NAME, null);
+      resultList = storageStrategy.getBlobKeysInsideContainer(CONTAINER_NAME, null, null);
       assertNotNull(resultList, "Result is null");
       assertFalse(resultList.iterator().hasNext(), "Blobs detected");
 
@@ -609,10 +609,10 @@ public void testGetBlobKeysInsideContainer() throws IOException {
                TestUtils.createRandomBlobKey("GetBlobKeys-", ".jpg"),
                TestUtils.createRandomBlobKey("563" + "/" + "g3sx2" + "/" + "removeBlob-", ".jpg"),
                TestUtils.createRandomBlobKey("563" + "/" + "g3sx2" + "/" + "removeBlob-", ".jpg") });
-      storageStrategy.getBlobKeysInsideContainer(CONTAINER_NAME, null);
+      storageStrategy.getBlobKeysInsideContainer(CONTAINER_NAME, null, null);
 
       List<String> retrievedBlobKeys = Lists.newArrayList();
-      resultList = storageStrategy.getBlobKeysInsideContainer(CONTAINER_NAME, null);
+      resultList = storageStrategy.getBlobKeysInsideContainer(CONTAINER_NAME, null, null);
       Iterator<String> containersIterator = resultList.iterator();
       while (containersIterator.hasNext()) {
          retrievedBlobKeys.add(containersIterator.next());

diff --git a/blobstore/src/main/java/org/jclouds/blobstore/LocalStorageStrategy.java b/blobstore/src/main/java/org/jclouds/blobstore/LocalStorageStrategy.java
@@ -98,7 +98,7 @@ public interface LocalStorageStrategy {
      * @return
      * @throws IOException
      */
-    Iterable<String> getBlobKeysInsideContainer(String container, String prefix) throws IOException;
+    Iterable<String> getBlobKeysInsideContainer(String container, String prefix, String delimiter) throws IOException;
 
     /**
      * Load the blob with the given key belonging to the container with the given

diff --git a/blobstore/src/main/java/org/jclouds/blobstore/TransientStorageStrategy.java b/blobstore/src/main/java/org/jclouds/blobstore/TransientStorageStrategy.java
@@ -150,7 +150,7 @@ public boolean blobExists(final String containerName, final String blobName) {
    }
 
    @Override
-   public Iterable<String> getBlobKeysInsideContainer(final String containerName, String prefix) {
+   public Iterable<String> getBlobKeysInsideContainer(final String containerName, String prefix, String delimiter) {
       ConcurrentSkipListMap<String, Blob> blobs = containerToBlobs.get(containerName);
       if (prefix == null) {
          return blobs.keySet();

diff --git a/blobstore/src/main/java/org/jclouds/blobstore/config/LocalBlobStore.java b/blobstore/src/main/java/org/jclouds/blobstore/config/LocalBlobStore.java
@@ -238,20 +238,12 @@ public PageSet<? extends StorageMetadata> list(final String containerName, ListC
       // Loading blobs from container
       Iterable<String> blobBelongingToContainer = null;
       try {
-         blobBelongingToContainer = storageStrategy.getBlobKeysInsideContainer(containerName, options.getPrefix());
+         blobBelongingToContainer = storageStrategy.getBlobKeysInsideContainer(containerName, options.getPrefix(), options.getDelimiter());
       } catch (IOException e) {
          logger.error(e, "An error occurred loading blobs contained into container %s", containerName);
          propagate(e);
       }
 
-      blobBelongingToContainer = Iterables.filter(blobBelongingToContainer,
-            new Predicate<String>() {
-               @Override
-               public boolean apply(String key) {
-                  // ignore folders
-                  return storageStrategy.blobExists(containerName, key);
-               }
-            });
       SortedSet<StorageMetadata> contents = newTreeSet(FluentIterable.from(blobBelongingToContainer)
             .transform(new Function<String, StorageMetadata>() {
                @Override
@@ -414,7 +406,7 @@ public boolean deleteContainerIfEmpty(String containerName) {
       boolean returnVal = true;
       if (storageStrategy.containerExists(containerName)) {
          try {
-            if (Iterables.isEmpty(storageStrategy.getBlobKeysInsideContainer(containerName, null)))
+            if (Iterables.isEmpty(storageStrategy.getBlobKeysInsideContainer(containerName, null, null)))
                storageStrategy.deleteContainer(containerName);
             else
                returnVal = false;