jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Gaul <notificati...@github.com>
Subject Re: [jclouds/jclouds] Fix for FileSystem blob store clearContainer with options (#1258)
Date Fri, 04 Jan 2019 00:32:37 GMT
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
Mime
View raw message