You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by reptillicus <no...@github.com> on 2018/11/20 01:50:23 UTC
[jclouds/jclouds] Fix for FileSystem blob store clearContainer with
options (#1258)
- Added a new `isDirEmpty` that uses an iterator to check emptiness, should be more performant for folders with many files
- Modified unit tests, cannot use the count of objects like in S3 for FileSystem blob store as directories are also counted individually
- Modified the logic in `clearContainer` when prefix, inDirectory, recursive are used. Its a bit messy in there, comments appreciated.
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds/pull/1258
-- Commit Summary --
* Fix for FileSystem blob store clearContainer with options
-- File Changes --
M apis/filesystem/src/main/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImpl.java (90)
M apis/filesystem/src/test/java/org/jclouds/filesystem/integration/FilesystemContainerIntegrationTest.java (3)
M blobstore/src/test/java/org/jclouds/blobstore/integration/internal/BaseContainerIntegrationTest.java (28)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/1258.patch
https://github.com/jclouds/jclouds/pull/1258.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/1258
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by reptillicus <no...@github.com>.
Ubuntu
On Wed, Nov 21, 2018, 3:09 PM Andrew Phillips <notifications@github.com
wrote:
> Odd, all the apis/filesystem tests passed on my machine.
>
> What OS are you running on?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/jclouds/jclouds/pull/1258#issuecomment-440843431>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/ABXuOs-ZZvYV2qUEHbcSA7spT63doQuqks5uxd1GgaJpZM4YqOLU>
> .
>
--
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/1258#issuecomment-440844070
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by reptillicus <no...@github.com>.
Just saw these, will update the PR over the break. Thanks!
--
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/1258#issuecomment-449581171
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by reptillicus <no...@github.com>.
reptillicus commented on this pull request.
> @@ -885,17 +932,26 @@ private void removeDirectoriesTreeOfBlobKey(String container, String blobKey) {
logger.debug("Could not look for attributes from %s: %s", directory, e);
}
- String[] children = directory.list();
- if (null == children || children.length == 0) {
- try {
- delete(directory);
- } catch (IOException e) {
- logger.debug("Could not delete %s: %s", directory, e);
- return;
+
+// String[] children = directory.list();
+// if (null == children || children.length == 0) {
done
--
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/1258#discussion_r245169301
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by Andrew Phillips <no...@github.com>.
Reopened #1258.
--
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/1258#event-1981288563
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by Andrew Phillips <no...@github.com>.
> Good to go from your perspectives?
@gaul @nacx Quick follow-up ping here...
--
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/1258#issuecomment-445684855
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by reptillicus <no...@github.com>.
@reptillicus pushed 1 commit.
f01663214c1b6eea23bbfaed431d4b9e9e3da5d7 fixes from comments
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1258/files/75b828fd20a1900fd45d39a3e93383283a9d2804..f01663214c1b6eea23bbfaed431d4b9e9e3da5d7
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by reptillicus <no...@github.com>.
reptillicus commented on this pull request.
> @@ -196,6 +196,7 @@ public void testSetContainerAccess() throws Exception {
@Override
public void testClearWithOptions() throws InterruptedException {
- throw new SkipException("filesystem does not support clear with options");
+// throw new SkipException("filesystem does not support clear with options");
+ super.testClearWithOptions();
Yeah it was there just for testing purposes to easily run that single test.
--
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/1258#discussion_r245150769
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by reptillicus <no...@github.com>.
reptillicus commented on this pull request.
> @@ -254,15 +256,48 @@ public void clearContainer(final String container) {
public void clearContainer(String container, ListContainerOptions options) {
filesystemContainerNameValidator.validate(container);
// TODO: these require calling removeDirectoriesTreeOfBlobKey
- checkArgument(options.getDir() == null && options.getPrefix() == null, "cannot specify directory or prefix");
+ String optsPrefix;
+ // TODO: Pick whichever one is not null? Not sure what to do until inDirectory is deprecated.
Looking into this at the moment, can we just check for both and do a `Throwables.propagate(IOException)` here?
--
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/1258#discussion_r245350387
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by Ignasi Barrera <no...@github.com>.
Failures in the PR build are due to an incorrect encoding configuration of Jenkins. I've fixed that and hopefully next one will succeed. Closing and reopening to trigger the build again.
--
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/1258#issuecomment-441058306
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by reptillicus <no...@github.com>.
reptillicus commented on this pull request.
> @@ -195,39 +197,55 @@ public void testClearWithOptions() throws InterruptedException {
options.prefix("path/1/2/3");
options.recursive();
view.getBlobStore().clearContainer(containerName, options);
- assertConsistencyAwareContainerSize(containerName, 2);
+ view.getBlobStore().countBlobs(containerName);
removed
--
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/1258#discussion_r245169259
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by Andrew Gaul <no...@github.com>.
Closed #1258.
--
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/1258#event-2055495367
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by Andrew Gaul <no...@github.com>.
gaul requested changes on this pull request.
Sorry for my tardiness; I was traveling for a few weeks.
> @@ -885,17 +932,26 @@ private void removeDirectoriesTreeOfBlobKey(String container, String blobKey) {
logger.debug("Could not look for attributes from %s: %s", directory, e);
}
- String[] children = directory.list();
- if (null == children || children.length == 0) {
- try {
- delete(directory);
- } catch (IOException e) {
- logger.debug("Could not delete %s: %s", directory, e);
- return;
+
+// String[] children = directory.list();
+// if (null == children || children.length == 0) {
Remove stale code.
> try {
- File containerFile = openFolder(container);
- File[] children = containerFile.listFiles();
- if (null != children) {
- for (File child : children)
- if (options.isRecursive() || child.isFile()) {
- Utils.deleteRecursively(child);
+ File object = new File(basePath);
+ if (object.isFile()) {
+ // To mimic the S3 type blobstores, a prefix for an object blob
+ // should also get deleted
+ delete(object);
+ }
+ else if (object.isDirectory() & (optsPrefix.endsWith(File.separator) | isNullOrEmpty(optsPrefix))) {
Prefer short-circuit boolean `&&` and `||`.
> - if (null == children || children.length == 0) {
- try {
- delete(directory);
- } catch (IOException e) {
- logger.debug("Could not delete %s: %s", directory, e);
- return;
+
+// String[] children = directory.list();
+// if (null == children || children.length == 0) {
+ // Don't need to do a listing on the dir, which could be costly. The iterator should be more performant.
+ try {
+ if (isDirEmpty(directory.getPath())) {
+ try {
+ delete(directory);
+ } catch (IOException e) {
+ logger.debug("Could not delete %s: %s", directory, e);
Should this throw an `IOException` instead, possibly wrapped in a `RuntimeException` if necessary? Repeated below.
> @@ -196,6 +196,7 @@ public void testSetContainerAccess() throws Exception {
@Override
public void testClearWithOptions() throws InterruptedException {
- throw new SkipException("filesystem does not support clear with options");
+// throw new SkipException("filesystem does not support clear with options");
+ super.testClearWithOptions();
Remove this method entirely? The subclass method will get called automatically.
> @@ -195,39 +197,55 @@ public void testClearWithOptions() throws InterruptedException {
options.prefix("path/1/2/3");
options.recursive();
view.getBlobStore().clearContainer(containerName, options);
- assertConsistencyAwareContainerSize(containerName, 2);
+ view.getBlobStore().countBlobs(containerName);
Why call `countBlobs` if you don't check the value? Repeated below.
> + // should also get deleted
+ delete(object);
+ }
+ else if (object.isDirectory() & (optsPrefix.endsWith(File.separator) | isNullOrEmpty(optsPrefix))) {
+ // S3 blobstores will only match prefixes that end with a trailing slash/file separator
+ // For insance, if we have a blob at /path/1/2/a, a prefix of /path/1/2 will not list /path/1/2/a
+ // but a prefix of /path/1/2/ will
+ File containerFile = openFolder(container + File.separator + normalizedOptsPath);
+ File[] children = containerFile.listFiles();
+ if (null != children) {
+ for (File child : children) {
+ if (options.isRecursive()) {
+ Utils.deleteRecursively(child);
+ } else {
+ if (child.isFile()) {
+ Utils.delete(child);
It seems like both this and `removeDirectoriesTreeOfBlobKey` have similar logic -- can you consolidate these?
> @@ -254,15 +255,49 @@ public void clearContainer(final String container) {
public void clearContainer(String container, ListContainerOptions options) {
filesystemContainerNameValidator.validate(container);
// TODO: these require calling removeDirectoriesTreeOfBlobKey
- checkArgument(options.getDir() == null && options.getPrefix() == null, "cannot specify directory or prefix");
+// checkArgument(options.getDir() == null && options.getPrefix() == null, "cannot specify directory or prefix");
Remove commented code.
> @@ -254,15 +255,49 @@ public void clearContainer(final String container) {
public void clearContainer(String container, ListContainerOptions options) {
filesystemContainerNameValidator.validate(container);
// TODO: these require calling removeDirectoriesTreeOfBlobKey
- checkArgument(options.getDir() == null && options.getPrefix() == null, "cannot specify directory or prefix");
+// checkArgument(options.getDir() == null && options.getPrefix() == null, "cannot specify directory or prefix");
+ String optsPrefix;
+ // Pick whichever one is not null? Not sure what to do until inDirectory is deprecated.
+ optsPrefix = options.getDir() == null ? options.getPrefix() : options.getDir();
+ // If both are null, just use an empty string
+ optsPrefix = optsPrefix == null ? "" : optsPrefix;
Can call `Strings.nullToEmpty`.
> @@ -254,15 +255,49 @@ public void clearContainer(final String container) {
public void clearContainer(String container, ListContainerOptions options) {
filesystemContainerNameValidator.validate(container);
// TODO: these require calling removeDirectoriesTreeOfBlobKey
- checkArgument(options.getDir() == null && options.getPrefix() == null, "cannot specify directory or prefix");
+// checkArgument(options.getDir() == null && options.getPrefix() == null, "cannot specify directory or prefix");
+ String optsPrefix;
+ // Pick whichever one is not null? Not sure what to do until inDirectory is deprecated.
Add `TODO:`.
> + File containerFile = openFolder(container + File.separator + normalizedOptsPath);
+ File[] children = containerFile.listFiles();
+ if (null != children) {
+ for (File child : children) {
+ if (options.isRecursive()) {
+ Utils.deleteRecursively(child);
+ } else {
+ if (child.isFile()) {
+ Utils.delete(child);
+ }
+ }
+ }
+ }
+
+ // Empty dirs in path if they don't have any objects
+ if (!isNullOrEmpty(optsPrefix)) {
Earlier you force `optsPrefix` to be non-null.
> @@ -867,6 +913,7 @@ private void removeDirectoriesTreeOfBlobKey(String container, String blobKey) {
File file = new File(normalizedBlobKey);
// TODO
// "/media/data/works/java/amazon/jclouds/master/filesystem/aa/bb/cc/dd/eef6f0c8-0206-460b-8870-352e6019893c.txt"
+
Stray modification.
--
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/1258#pullrequestreview-186387777
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by reptillicus <no...@github.com>.
reptillicus commented on this pull request.
> try {
- File containerFile = openFolder(container);
- File[] children = containerFile.listFiles();
- if (null != children) {
- for (File child : children)
- if (options.isRecursive() || child.isFile()) {
- Utils.deleteRecursively(child);
+ File object = new File(basePath);
+ if (object.isFile()) {
+ // To mimic the S3 type blobstores, a prefix for an object blob
+ // should also get deleted
+ delete(object);
+ }
+ else if (object.isDirectory() & (optsPrefix.endsWith(File.separator) | isNullOrEmpty(optsPrefix))) {
fixed
--
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/1258#discussion_r245169284
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by Andrew Phillips <no...@github.com>.
Closing and reopening to trigger Jenkins again
--
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/1258#issuecomment-440845569
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by Ashkan Paya <no...@github.com>.
Unsubscribe please.
From: reptillicus <no...@github.com>
Reply-To: jclouds/jclouds <re...@reply.github.com>
Date: Thursday, January 3, 2019 at 3:48 PM
To: jclouds/jclouds <jc...@noreply.github.com>
Cc: Subscribed <su...@noreply.github.com>
Subject: Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer with options (#1258)
[EXTERNAL EMAIL]
@reptillicus commented on this pull request.
________________________________
In apis/filesystem/src/main/java/org/jclouds/filesystem/strategy/internal/FilesystemStorageStrategyImpl.java<https://github.com/jclouds/jclouds/pull/1258#discussion_r245169336>:
> @@ -254,15 +255,49 @@ public void clearContainer(final String container) {
public void clearContainer(String container, ListContainerOptions options) {
filesystemContainerNameValidator.validate(container);
// TODO: these require calling removeDirectoriesTreeOfBlobKey
- checkArgument(options.getDir() == null && options.getPrefix() == null, "cannot specify directory or prefix");
+// checkArgument(options.getDir() == null && options.getPrefix() == null, "cannot specify directory or prefix");
+ String optsPrefix;
+ // Pick whichever one is not null? Not sure what to do until inDirectory is deprecated.
+ optsPrefix = options.getDir() == null ? options.getPrefix() : options.getDir();
+ // If both are null, just use an empty string
+ optsPrefix = optsPrefix == null ? "" : optsPrefix;
changed
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub<https://github.com/jclouds/jclouds/pull/1258#discussion_r245169336>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AD--O2oLmummCoYBaov05oWkX1grPEW9ks5u_pa8gaJpZM4YqOLU>.
--
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/1258#issuecomment-451314978
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by reptillicus <no...@github.com>.
reptillicus commented on this pull request.
> - if (null == children || children.length == 0) {
- try {
- delete(directory);
- } catch (IOException e) {
- logger.debug("Could not delete %s: %s", directory, e);
- return;
+
+// String[] children = directory.list();
+// if (null == children || children.length == 0) {
+ // Don't need to do a listing on the dir, which could be costly. The iterator should be more performant.
+ try {
+ if (isDirEmpty(directory.getPath())) {
+ try {
+ delete(directory);
+ } catch (IOException e) {
+ logger.debug("Could not delete %s: %s", directory, e);
I believe that the s3 type blob store basically just does nothing when the delete doesn't happen, I'm guessing thats there to be compliant with that?
--
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/1258#discussion_r245150673
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by Andrew Phillips <no...@github.com>.
> Odd, all the apis/filesystem tests passed on my machine.
What OS are you running on?
--
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/1258#issuecomment-440843431
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by Andrew Gaul <no...@github.com>.
Squashed, made some style edits, and pushed to master as a36c9dcef06f14d26850c2cf3f3a661aa303914b and 2.1.x as e118e5856736f22dc049fd950318e5b2959e25d5. Thank you for your contribution @reptillicus!
--
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/1258#issuecomment-451587528
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by Andrew Phillips <no...@github.com>.
@gaul @nacx Good to go from your perspectives?
--
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/1258#issuecomment-443530176
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by Andrew Phillips <no...@github.com>.
Closed #1258.
--
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/1258#event-1981288517
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by reptillicus <no...@github.com>.
reptillicus commented on this pull request.
> @@ -254,15 +255,49 @@ public void clearContainer(final String container) {
public void clearContainer(String container, ListContainerOptions options) {
filesystemContainerNameValidator.validate(container);
// TODO: these require calling removeDirectoriesTreeOfBlobKey
- checkArgument(options.getDir() == null && options.getPrefix() == null, "cannot specify directory or prefix");
+// checkArgument(options.getDir() == null && options.getPrefix() == null, "cannot specify directory or prefix");
+ String optsPrefix;
+ // Pick whichever one is not null? Not sure what to do until inDirectory is deprecated.
+ optsPrefix = options.getDir() == null ? options.getPrefix() : options.getDir();
+ // If both are null, just use an empty string
+ optsPrefix = optsPrefix == null ? "" : optsPrefix;
changed
--
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/1258#discussion_r245169336
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by Andrew Phillips <no...@github.com>.
> Ubuntu
Ah, interesting - not obvious why Jenkins would fail, in that case. Let's try that again.
Here two failures when running on Windows:
```
Tests run: 232, Failures: 2, Errors: 0, Skipped: 221, Time elapsed: 6.339 sec <<
< FAILURE! - in TestSuite
tearDown(org.jclouds.filesystem.FilesystemBlobStoreTest) Time elapsed: 0.368 se
c <<< FAILURE!
java.nio.file.FileSystemException: .\target\basedir\fun-blobstore-test\testkey-8
89c002f-cd20-4524-9951-3f49446b10a4.jpg: The process cannot access the file beca
use it is being used by another process.
at sun.nio.fs.WindowsException.translateToIOException(WindowsException.j
ava:86)
at sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.jav
a:97)
at sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.jav
a:102)
at sun.nio.fs.WindowsFileSystemProvider.implDelete(WindowsFileSystemProv
ider.java:269)
at sun.nio.fs.AbstractFileSystemProvider.delete(AbstractFileSystemProvid
er.java:103)
at java.nio.file.Files.delete(Files.java:1077)
at org.jclouds.filesystem.util.Utils.delete(Utils.java:90)
at org.jclouds.filesystem.util.Utils.deleteRecursively(Utils.java:77)
at org.jclouds.filesystem.util.Utils.deleteRecursively(Utils.java:72)
at org.jclouds.filesystem.util.Utils.deleteRecursively(Utils.java:72)
at org.jclouds.filesystem.FilesystemBlobStoreTest.tearDown(FilesystemBlo
bStoreTest.java:107)
setUp(org.jclouds.filesystem.strategy.internal.FilesystemStorageStrategyImplTest
) Time elapsed: 0.04 sec <<< FAILURE!
java.nio.file.FileSystemException: .\target\basedir\fun-blobstore-test\testkey-8
89c002f-cd20-4524-9951-3f49446b10a4.jpg: The process cannot access the file beca
use it is being used by another process.
at sun.nio.fs.WindowsException.translateToIOException(WindowsException.j
ava:86)
at sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.jav
a:97)
at sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.jav
a:102)
at sun.nio.fs.WindowsFileSystemProvider.implDelete(WindowsFileSystemProv
ider.java:269)
at sun.nio.fs.AbstractFileSystemProvider.delete(AbstractFileSystemProvid
er.java:103)
at java.nio.file.Files.delete(Files.java:1077)
at org.jclouds.filesystem.util.Utils.delete(Utils.java:90)
at org.jclouds.filesystem.util.Utils.deleteRecursively(Utils.java:77)
at org.jclouds.filesystem.util.Utils.deleteRecursively(Utils.java:72)
at org.jclouds.filesystem.utils.TestUtils.cleanDirectoryContent(TestUtil
s.java:172)
at org.jclouds.filesystem.strategy.internal.FilesystemStorageStrategyImp
lTest.setUp(FilesystemStorageStrategyImplTest.java:101)
Results :
Failed tests:
FilesystemBlobStoreTest.tearDown:107 ╗ FileSystem .\target\basedir\fun-blobsto
...
FilesystemStorageStrategyImplTest.setUp:101 ╗ FileSystem .\target\basedir\fun-
...
Tests run: 232, Failures: 2, Errors: 0, Skipped: 221
```
--
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/1258#issuecomment-440845320
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by reptillicus <no...@github.com>.
reptillicus commented on this pull request.
> + // should also get deleted
+ delete(object);
+ }
+ else if (object.isDirectory() & (optsPrefix.endsWith(File.separator) | isNullOrEmpty(optsPrefix))) {
+ // S3 blobstores will only match prefixes that end with a trailing slash/file separator
+ // For insance, if we have a blob at /path/1/2/a, a prefix of /path/1/2 will not list /path/1/2/a
+ // but a prefix of /path/1/2/ will
+ File containerFile = openFolder(container + File.separator + normalizedOptsPath);
+ File[] children = containerFile.listFiles();
+ if (null != children) {
+ for (File child : children) {
+ if (options.isRecursive()) {
+ Utils.deleteRecursively(child);
+ } else {
+ if (child.isFile()) {
+ Utils.delete(child);
It might be possible, but `removeDirectoriesTreeOfBlobKey` walks up the tree, and this one is walking deeper into the tree. Suggestions?
--
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/1258#discussion_r245168313
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by Ignasi Barrera <no...@github.com>.
Reopened #1258.
--
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/1258#event-1982735293
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by Ignasi Barrera <no...@github.com>.
Closed #1258.
--
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/1258#event-1982735204
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by Andrew Gaul <no...@github.com>.
gaul requested changes on this pull request.
> @@ -253,16 +255,52 @@ public void clearContainer(final String container) {
@Override
public void clearContainer(String container, ListContainerOptions options) {
filesystemContainerNameValidator.validate(container);
+ if (options.getDir() != null && options.getPrefix() != null) {
+ Throwables.propagate(new IOException("Cannot use both dir and prefix at the same time."));
This will read better as `Preconditions.checkArgument`.
> @@ -254,15 +256,48 @@ public void clearContainer(final String container) {
public void clearContainer(String container, ListContainerOptions options) {
filesystemContainerNameValidator.validate(container);
// TODO: these require calling removeDirectoriesTreeOfBlobKey
- checkArgument(options.getDir() == null && options.getPrefix() == null, "cannot specify directory or prefix");
+ String optsPrefix;
+ // TODO: Pick whichever one is not null? Not sure what to do until inDirectory is deprecated.
Need to remove the stale TODO.
> + // should also get deleted
+ delete(object);
+ }
+ else if (object.isDirectory() & (optsPrefix.endsWith(File.separator) | isNullOrEmpty(optsPrefix))) {
+ // S3 blobstores will only match prefixes that end with a trailing slash/file separator
+ // For insance, if we have a blob at /path/1/2/a, a prefix of /path/1/2 will not list /path/1/2/a
+ // but a prefix of /path/1/2/ will
+ File containerFile = openFolder(container + File.separator + normalizedOptsPath);
+ File[] children = containerFile.listFiles();
+ if (null != children) {
+ for (File child : children) {
+ if (options.isRecursive()) {
+ Utils.deleteRecursively(child);
+ } else {
+ if (child.isFile()) {
+ Utils.delete(child);
I guess this is the best we can do.
--
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/1258#pullrequestreview-189471713
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by Andrew Gaul <no...@github.com>.
gaul requested changes on this pull request.
Looks good, please address small comments.
> - if (null == children || children.length == 0) {
- try {
- delete(directory);
- } catch (IOException e) {
- logger.debug("Could not delete %s: %s", directory, e);
- return;
+
+// String[] children = directory.list();
+// if (null == children || children.length == 0) {
+ // Don't need to do a listing on the dir, which could be costly. The iterator should be more performant.
+ try {
+ if (isDirEmpty(directory.getPath())) {
+ try {
+ delete(directory);
+ } catch (IOException e) {
+ logger.debug("Could not delete %s: %s", directory, e);
Sorry I missed that this is code movement. I do think a more narrow exception handling for just removing non-existent files is better but outside the scope of this pull request.
> @@ -254,15 +256,48 @@ public void clearContainer(final String container) {
public void clearContainer(String container, ListContainerOptions options) {
filesystemContainerNameValidator.validate(container);
// TODO: these require calling removeDirectoriesTreeOfBlobKey
- checkArgument(options.getDir() == null && options.getPrefix() == null, "cannot specify directory or prefix");
+ String optsPrefix;
+ // TODO: Pick whichever one is not null? Not sure what to do until inDirectory is deprecated.
I think it would be OK to throw an exception if both are set.
> }
- // recursively call for removing other path
- removeDirectoriesTreeOfBlobKey(container, parentPath);
+ } catch (IOException e ) {
Stray whitespace after `e`.
--
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/1258#pullrequestreview-189233154
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by reptillicus <no...@github.com>.
@reptillicus pushed 1 commit.
882eb18cc8cb6cd261125190e02fc0c166686730 Added Throwables.propagate() to the clearContainer method when both dir and prefix are set
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1258/files/f01663214c1b6eea23bbfaed431d4b9e9e3da5d7..882eb18cc8cb6cd261125190e02fc0c166686730
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by reptillicus <no...@github.com>.
Odd, all the apis/filesystem tests passed on my machine.
--
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/1258#issuecomment-440749536
Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer
with options (#1258)
Posted by reptillicus <no...@github.com>.
@reptillicus pushed 2 commits.
09d286627ae5ac5a0ef090d6b504c72ab3c6b00c Fixes from comments on PR
c307e78948620c125594d99e0d73f7c9f06d71e7 Merge branch 'master' of github.com:jclouds/jclouds into fix/filesystemstore
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1258/files/882eb18cc8cb6cd261125190e02fc0c166686730..c307e78948620c125594d99e0d73f7c9f06d71e7