[GitHub] [brooklyn-server] jcabrerizo commented on a diff in pull request #1322: fix most reliances on deleteOnExit, deleting when done with the file

[GitBox]

jcabrerizo commented on code in PR #1322:
URL: https://github.com/apache/brooklyn-server/pull/1322#discussion_r906232865


##########
core/src/main/java/org/apache/brooklyn/core/mgmt/ha/BrooklynBomOsgiArchiveInstaller.java:
##########
@@ -96,8 +96,21 @@ public class BrooklynBomOsgiArchiveInstaller {
     private boolean force = false;
     private boolean deferredStart = false;
     private boolean validateTypes = true;
-    
-    private File zipFile;
+
+    public static class FileWithTempInfo<T extends File> {
+        public FileWithTempInfo(T f, boolean isTemp) { this.f = f; this.isTemp 
= isTemp; }
+        public T f;

Review Comment:
   I think `f` should be final to avoid it to be replaced. 
   It's not very importand, but I'd feel more comfortable if instead of been 
public, it's exposed by a getter method, it also will make this code more clear:
   
   ```
   FileWithTempInfo tempFile = new FileWithTempInfo(file, true);
   
   ...
   tempFile.getFile();
   _vs_
   tempFile.f;
   ```
   



##########
software/base/src/main/java/org/apache/brooklyn/entity/brooklynnode/BrooklynNodeSshDriver.java:
##########
@@ -261,8 +262,10 @@ public void customize() {
             checkNotNull(url, "url");
 
             // If a local folder, then create archive from contents first
+            List<File> filesToDelete = MutableList.of();

Review Comment:
   Same as above 



##########
core/src/main/java/org/apache/brooklyn/util/core/file/ArchiveUtils.java:
##########
@@ -213,8 +213,10 @@ public static void deploy(String archiveUrl, 
SshMachineLocation machine, String
      * @see #deploy(Map, String, SshMachineLocation, String, String, String)
      */
     public static void deploy(Map<String, ?> props, String archiveUrl, 
SshMachineLocation machine, String destDir) {
+        List<File> filesToDelete = MutableList.of();

Review Comment:
   I  can't see why this is is a list of files if it only could content one zip 
file.
   This will a bit faster and consume less memory
   ```
           File toDelete = null;
           if (Urls.isDirectory(archiveUrl)) {
               File zipFile = ArchiveBuilder.zip().entry(".", 
Urls.toFile(archiveUrl)).create();
               toDelete = zipFile;
               archiveUrl = zipFile.getAbsolutePath();
           }
           ...
           if(toDelete != null) toDelete.delete();
   ```



##########
software/base/src/main/java/org/apache/brooklyn/entity/java/VanillaJavaAppSshDriver.java:
##########
@@ -85,9 +85,11 @@ public void customize() {
         SshMachineLocation machine = getMachine();
         VanillaJavaApp entity = getEntity();
         for (String entry : entity.getClasspath()) {
+            List<File> filesToDelete = MutableList.of();

Review Comment:
   Same as above



##########
core/src/main/java/org/apache/brooklyn/util/core/file/ArchiveBuilder.java:
##########
@@ -44,6 +44,7 @@
 import com.google.common.collect.LinkedHashMultimap;
 import com.google.common.collect.Multimap;
 import com.google.common.io.Files;
+import org.apache.brooklyn.util.time.Duration;

Review Comment:
   This new import is not used



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@brooklyn.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org