Re: [jclouds/jclouds] JCLOUDS-1371: JCLOUDS-1488: list optimize prefix (#1268)

[Andrew Phillips]

demobox commented on this pull request.



> @@ -148,8 +149,13 @@ public boolean blobExists(final String containerName, 
> final String blobName) {
}
 
@Override
-   public Iterable getBlobKeysInsideContainer(final String 
containerName) {
-  return containerToBlobs.get(containerName).keySet();
+   public Iterable getBlobKeysInsideContainer(final String 
containerName, String prefix) {
+  ConcurrentSkipListMap blobs = 
containerToBlobs.get(containerName);
+  if (prefix == null) {
+ return blobs.keySet();
+  }
+  String lastPrefix = prefix + (char) 65535;  // TODO: better sentinel?

[minor] Perhaps create a constant `SENTINEL` or so just to make it clear that 
that's what the last character is supposed to be?

That might be confusing, especially if the TODO is removed in future?

-- 
You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1268#discussion_r252093248

Re: [jclouds/jclouds] JCLOUDS-1371: JCLOUDS-1488: list optimize prefix (#1268)

[Andrew Gaul]

gaul commented on this pull request.



>   } else if (child.isDirectory()) {
-blobNames.add(function.apply(child.getAbsolutePath()) + 
File.separator); // TODO: undo if failures
-populateBlobKeysInContainer(child, blobNames, function);
+// Consider a prefix /a/b/c but we have only descended to path /a.
+// We need to match the path against the prefix to continue

Addressed suggestion via a more narrow filter.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1268#discussion_r252090379

Re: [jclouds/jclouds] JCLOUDS-1371: JCLOUDS-1488: list optimize prefix (#1268)

[Timur Alperovich]

timuralp approved this pull request.

Looks good to me!

>   } else if (child.isDirectory()) {
-blobNames.add(function.apply(child.getAbsolutePath()) + 
File.separator); // TODO: undo if failures
-populateBlobKeysInContainer(child, blobNames, function);
+// Consider a prefix /a/b/c but we have only descended to path /a.
+// We need to match the path against the prefix to continue

I was confused by the additional check here, but after removing it and running 
the tests, it makes sense: the directories will not include the trailing "/", 
so if the prefix is "dir/", then it wouldn't match the directory "dir" 
(excluding it from the list and not listing its contents). I think this comment 
helps, but maybe it could more explicitly call out this specific issues. As far 
as I can tell, this only affects prefix listings that end with the file 
separator.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1268#pullrequestreview-197775752

Re: [jclouds/jclouds] JCLOUDS-1371: JCLOUDS-1488: list optimize prefix (#1268)

[Ignasi Barrera]

nacx approved this pull request.





-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1268#pullrequestreview-197508785

Re: [jclouds/jclouds] JCLOUDS-1371: JCLOUDS-1488: list optimize prefix (#1268)

[Andrew Gaul]

@timuralp could you review this?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1268#issuecomment-457725188

Re: [jclouds/jclouds] JCLOUDS-1371: JCLOUDS-1488: list optimize prefix (#1268)

[Andrew Gaul]

gaul commented on this pull request.



> @@ -148,8 +149,13 @@ public boolean blobExists(final String containerName, 
> final String blobName) {
}
 
@Override
-   public Iterable getBlobKeysInsideContainer(final String 
containerName) {
-  return containerToBlobs.get(containerName).keySet();
+   public Iterable getBlobKeysInsideContainer(final String 
containerName, String prefix) {
+  ConcurrentSkipListMap blobs = 
containerToBlobs.get(containerName);
+  if (prefix == null) {
+ return blobs.keySet();
+  }
+  String lastPrefix = prefix + (char) 65535;  // TODO: better sentinel?

This is hacky but I don't think is has practical drawbacks?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1268#pullrequestreview-196707281

[jclouds/jclouds] JCLOUDS-1371: JCLOUDS-1488: list optimize prefix (#1268)

[Andrew Gaul]

Previously getBlobKeysInsideContainer returned all keys and filtered
in LocalBlobStore.  Now getBlobKeysInsideContainer filters via prefix
which can dramatically decrease the number of keys returned,
especially for the filesystem provider.  Further optimizations are
possible for delimiter.
You can view, comment on, or merge this pull request online at:

  https://github.com/jclouds/jclouds/pull/1268

-- Commit Summary --

  * JCLOUDS-1371: JCLOUDS-1488: list optimize prefix

-- File Changes --

M 
apis/filesystem/src/main/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImpl.java
 (18)
M 
apis/filesystem/src/test/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImplTest.java
 (8)
M blobstore/src/main/java/org/jclouds/blobstore/LocalStorageStrategy.java 
(2)
M 
blobstore/src/main/java/org/jclouds/blobstore/TransientStorageStrategy.java (14)
M blobstore/src/main/java/org/jclouds/blobstore/config/LocalBlobStore.java 
(4)

-- Patch Links --

https://github.com/jclouds/jclouds/pull/1268.patch
https://github.com/jclouds/jclouds/pull/1268.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1268