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
