[GitHub] [spark] srowen commented on a diff in pull request #36529: [SPARK-39102][CORE][SQL][DSTREAM] Add checkstyle rules to disabled use of Guava's `Files.createTempDir()`

2023-03-30 Thread via GitHub


srowen commented on code in PR #36529:
URL: https://github.com/apache/spark/pull/36529#discussion_r1153918827


##
common/network-common/src/main/java/org/apache/spark/network/util/JavaUtils.java:
##
@@ -362,6 +364,60 @@ public static byte[] bufferToArray(ByteBuffer buffer) {
 }
   }
 
+  /**
+   * Create a temporary directory inside `java.io.tmpdir` with default 
namePrefix "spark".
+   * The directory will be automatically deleted when the VM shuts down.
+   */
+  public static File createTempDir() throws IOException {
+return createTempDir(System.getProperty("java.io.tmpdir"), "spark");
+  }
+
+  /**
+   * Create a temporary directory inside the given parent directory. The 
directory will be
+   * automatically deleted when the VM shuts down.
+   */
+  public static File createTempDir(String root, String namePrefix) throws 
IOException {

Review Comment:
   I'm not sure about a revert. I'm not sure there is even a problem here.



-- 
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: reviews-unsubscr...@spark.apache.org

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


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] [spark] srowen commented on a diff in pull request #36529: [SPARK-39102][CORE][SQL][DSTREAM] Add checkstyle rules to disabled use of Guava's `Files.createTempDir()`

2023-03-30 Thread via GitHub


srowen commented on code in PR #36529:
URL: https://github.com/apache/spark/pull/36529#discussion_r1153918687


##
common/network-common/src/main/java/org/apache/spark/network/util/JavaUtils.java:
##
@@ -362,6 +364,60 @@ public static byte[] bufferToArray(ByteBuffer buffer) {
 }
   }
 
+  /**
+   * Create a temporary directory inside `java.io.tmpdir` with default 
namePrefix "spark".
+   * The directory will be automatically deleted when the VM shuts down.
+   */
+  public static File createTempDir() throws IOException {
+return createTempDir(System.getProperty("java.io.tmpdir"), "spark");
+  }
+
+  /**
+   * Create a temporary directory inside the given parent directory. The 
directory will be
+   * automatically deleted when the VM shuts down.
+   */
+  public static File createTempDir(String root, String namePrefix) throws 
IOException {

Review Comment:
   Oh, OK if that means moving more code around, can we just adjust the 
Utils.createTempDir implementation? separate it from JavaUtils.createTempDir I 
guess, which can still be used for test code (or moved to the test tree), or 
maybe we don't need both



-- 
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: reviews-unsubscr...@spark.apache.org

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


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] [spark] srowen commented on a diff in pull request #36529: [SPARK-39102][CORE][SQL][DSTREAM] Add checkstyle rules to disabled use of Guava's `Files.createTempDir()`

2023-03-30 Thread via GitHub


srowen commented on code in PR #36529:
URL: https://github.com/apache/spark/pull/36529#discussion_r1153736049


##
common/network-common/src/main/java/org/apache/spark/network/util/JavaUtils.java:
##
@@ -362,6 +364,60 @@ public static byte[] bufferToArray(ByteBuffer buffer) {
 }
   }
 
+  /**
+   * Create a temporary directory inside `java.io.tmpdir` with default 
namePrefix "spark".
+   * The directory will be automatically deleted when the VM shuts down.
+   */
+  public static File createTempDir() throws IOException {
+return createTempDir(System.getProperty("java.io.tmpdir"), "spark");
+  }
+
+  /**
+   * Create a temporary directory inside the given parent directory. The 
directory will be
+   * automatically deleted when the VM shuts down.
+   */
+  public static File createTempDir(String root, String namePrefix) throws 
IOException {

Review Comment:
   Let's just make everything use the explicit shutdown hook mechanism? is 
there any downside? Like change the JavaUtils 'base' method to just do that.



-- 
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: reviews-unsubscr...@spark.apache.org

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


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] [spark] srowen commented on a diff in pull request #36529: [SPARK-39102][CORE][SQL][DSTREAM] Add checkstyle rules to disabled use of Guava's `Files.createTempDir()`

2023-03-29 Thread via GitHub


srowen commented on code in PR #36529:
URL: https://github.com/apache/spark/pull/36529#discussion_r1152590107


##
common/network-common/src/main/java/org/apache/spark/network/util/JavaUtils.java:
##
@@ -362,6 +364,60 @@ public static byte[] bufferToArray(ByteBuffer buffer) {
 }
   }
 
+  /**
+   * Create a temporary directory inside `java.io.tmpdir` with default 
namePrefix "spark".
+   * The directory will be automatically deleted when the VM shuts down.
+   */
+  public static File createTempDir() throws IOException {
+return createTempDir(System.getProperty("java.io.tmpdir"), "spark");
+  }
+
+  /**
+   * Create a temporary directory inside the given parent directory. The 
directory will be
+   * automatically deleted when the VM shuts down.
+   */
+  public static File createTempDir(String root, String namePrefix) throws 
IOException {

Review Comment:
   It's used in non-test code. What's the issue with deleteOnExit, or is there 
a better way to ensure tmp dirs are cleaned up?



-- 
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: reviews-unsubscr...@spark.apache.org

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


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org