You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/05/19 12:22:20 UTC
[GitHub] [spark] LuciferYang opened a new pull request, #36611: [SPARK-39204][CORE][SQL] Replace `Utils.createTempDir` with `JavaUtils.createTempDir`
LuciferYang opened a new pull request, #36611:
URL: https://github.com/apache/spark/pull/36611
### What changes were proposed in this pull request?
This main change of this pr is replace all use of `Utils.createTempDir` with `JavaUtils.createTempDir`, the replacement rules are as follows:
- `Utils.createTempDir()` -> `JavaUtils.createTempDir()`
- `Utils.createTempDir(rootDir)` and `Utils.createTempDir(root = rootDir)` -> `JavaUtils.createTempDirWithRoot(rootDir)`
- `Utils.createTempDir(namePrefix = prefix)` -> `JavaUtils.createTempDirWithPrefix(prefix)`
- `Utils.createTempDir(rootDir, prefix)` -> `JavaUtils.createTempDir(rootDir, prefix)`
Another change is to delete `Utils.createTempDir()` method to keep only one `createTempDir()` method.
### Why are the changes needed?
Keep only one `createTempDir()` method in Spark.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Pass GitHub Action.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #36611: [SPARK-39204][BUILD][CORE][SQL][DSTREAM][GRAPHX][K8S][ML][MLLIB][SS][YARN][EXAMPLES][SHELL] Replace `Utils.createTempDir` with `JavaUtils.createTempDir`
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on PR #36611:
URL: https://github.com/apache/spark/pull/36611#issuecomment-1132340311
Yeah, I think we should better fix `Utils.createTempDir`.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on a diff in pull request #36611: [SPARK-39204][CORE] Change `Utils.createTempDir` and `Utils.createDirectory` call the same logic method in `JavaUtils`
Posted by GitBox <gi...@apache.org>.
srowen commented on code in PR #36611:
URL: https://github.com/apache/spark/pull/36611#discussion_r878325944
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -308,28 +308,7 @@ private[spark] object Utils extends Logging {
* newly created, and is not marked for automatic deletion.
*/
def createDirectory(root: String, namePrefix: String = "spark"): File = {
- var attempts = 0
- val maxAttempts = MAX_DIR_CREATION_ATTEMPTS
- var dir: File = null
- while (dir == null) {
- attempts += 1
- if (attempts > maxAttempts) {
- throw new IOException("Failed to create a temp directory (under " + root + ") after " +
- maxAttempts + " attempts!")
- }
- try {
- dir = new File(root, namePrefix + "-" + UUID.randomUUID.toString)
- // SPARK-35907:
- // This could throw more meaningful exception information if directory creation failed.
- Files.createDirectories(dir.toPath)
- } catch {
- case e @ (_ : IOException | _ : SecurityException) =>
- logError(s"Failed to create directory $dir", e)
- dir = null
- }
- }
-
- dir.getCanonicalFile
+ JavaUtils.createDirectory(root, namePrefix)
Review Comment:
Ah right, that's fine
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on a diff in pull request #36611: [SPARK-39204][CORE] Change `Utils.createTempDir` and `Utils.createDirectory` call the same logic method in `JavaUtils`
Posted by GitBox <gi...@apache.org>.
srowen commented on code in PR #36611:
URL: https://github.com/apache/spark/pull/36611#discussion_r878304272
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -308,28 +308,7 @@ private[spark] object Utils extends Logging {
* newly created, and is not marked for automatic deletion.
*/
def createDirectory(root: String, namePrefix: String = "spark"): File = {
- var attempts = 0
- val maxAttempts = MAX_DIR_CREATION_ATTEMPTS
- var dir: File = null
- while (dir == null) {
- attempts += 1
- if (attempts > maxAttempts) {
- throw new IOException("Failed to create a temp directory (under " + root + ") after " +
- maxAttempts + " attempts!")
- }
- try {
- dir = new File(root, namePrefix + "-" + UUID.randomUUID.toString)
- // SPARK-35907:
- // This could throw more meaningful exception information if directory creation failed.
- Files.createDirectories(dir.toPath)
- } catch {
- case e @ (_ : IOException | _ : SecurityException) =>
- logError(s"Failed to create directory $dir", e)
- dir = null
- }
- }
-
- dir.getCanonicalFile
+ JavaUtils.createDirectory(root, namePrefix)
Review Comment:
Hm, why did we not just implement the logic here? rather than use JavaUtils? I forget if there was a reason
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen closed pull request #36611: [SPARK-39204][CORE] Change `Utils.createTempDir` and `Utils.createDirectory` call the same logic method in `JavaUtils`
Posted by GitBox <gi...@apache.org>.
srowen closed pull request #36611: [SPARK-39204][CORE] Change `Utils.createTempDir` and `Utils.createDirectory` call the same logic method in `JavaUtils`
URL: https://github.com/apache/spark/pull/36611
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #36611: [SPARK-39204][CORE] Change `Utils.createTempDir` and `Utils.createDirectory` call the same logic method in `JavaUtils`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36611:
URL: https://github.com/apache/spark/pull/36611#discussion_r878325387
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -308,28 +308,7 @@ private[spark] object Utils extends Logging {
* newly created, and is not marked for automatic deletion.
*/
def createDirectory(root: String, namePrefix: String = "spark"): File = {
- var attempts = 0
- val maxAttempts = MAX_DIR_CREATION_ATTEMPTS
- var dir: File = null
- while (dir == null) {
- attempts += 1
- if (attempts > maxAttempts) {
- throw new IOException("Failed to create a temp directory (under " + root + ") after " +
- maxAttempts + " attempts!")
- }
- try {
- dir = new File(root, namePrefix + "-" + UUID.randomUUID.toString)
- // SPARK-35907:
- // This could throw more meaningful exception information if directory creation failed.
- Files.createDirectories(dir.toPath)
- } catch {
- case e @ (_ : IOException | _ : SecurityException) =>
- logError(s"Failed to create directory $dir", e)
- dir = null
- }
- }
-
- dir.getCanonicalFile
+ JavaUtils.createDirectory(root, namePrefix)
Review Comment:
In order to solve
```
Avoid the use of Guava's Files.createTempDir() in Spark code due to [CVE-2020-8908](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-8908)
```
I added two compatible methods in `JavaUtils`.
> Hm, why did we not just implement the logic here? rather than use JavaUtils? I forget if there was a reason
Code in `network-common` and `network-shuffle` module cannot call `Utils. createDirectory ` or `Utils. createTempDir` in `core` module.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #36611: [SPARK-39204][CORE] Change `Utils.createTempDir` and `Utils.createDirectory` call the same logic method in `JavaUtils`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36611:
URL: https://github.com/apache/spark/pull/36611#discussion_r878325387
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -308,28 +308,7 @@ private[spark] object Utils extends Logging {
* newly created, and is not marked for automatic deletion.
*/
def createDirectory(root: String, namePrefix: String = "spark"): File = {
- var attempts = 0
- val maxAttempts = MAX_DIR_CREATION_ATTEMPTS
- var dir: File = null
- while (dir == null) {
- attempts += 1
- if (attempts > maxAttempts) {
- throw new IOException("Failed to create a temp directory (under " + root + ") after " +
- maxAttempts + " attempts!")
- }
- try {
- dir = new File(root, namePrefix + "-" + UUID.randomUUID.toString)
- // SPARK-35907:
- // This could throw more meaningful exception information if directory creation failed.
- Files.createDirectories(dir.toPath)
- } catch {
- case e @ (_ : IOException | _ : SecurityException) =>
- logError(s"Failed to create directory $dir", e)
- dir = null
- }
- }
-
- dir.getCanonicalFile
+ JavaUtils.createDirectory(root, namePrefix)
Review Comment:
In order to solve
```
Avoid the use of Guava's Files.createTempDir() in Spark code due to [CVE-2020-8908](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-8908)
```
I added two compatible methods in `JavaUtils`.
> Hm, why did we not just implement the logic here? rather than use JavaUtils? I forget if there was a reason
Code in `network-common` and `network-shuffle` module cannot call `Utils.createDirectory ` or `Utils. createTempDir` in `core` module.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #36611: [WIP][SPARK-39204][BUILD][CORE][SQL][DSTREAM][GRAPHX][K8S][ML][MLLIB][SS][YARN][EXAMPLES][SHELL] Replace `Utils.createTempDir` with `JavaUtils.createTempDir`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #36611:
URL: https://github.com/apache/spark/pull/36611#issuecomment-1131660071
cc @attilapiros as discussed in SPARK-39102(https://github.com/apache/spark/pull/36529#discussion_r872838034), this pr replace `Utils.createTempDir()` with `JavaUtils.createTempDir()` and only keeping the one in `JavaUtils`
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #36611: [SPARK-39204][BUILD][CORE][SQL][DSTREAM][GRAPHX][K8S][ML][MLLIB][SS][YARN][EXAMPLES][SHELL] Replace `Utils.createTempDir` with `JavaUtils.createTempDir`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #36611:
URL: https://github.com/apache/spark/pull/36611#issuecomment-1131979146
It seems that this change is big. Another way to keep one `createTempDir` is to let `Utils.createTempDir` call `JavaUtils.createTempDir` . Is this acceptable?
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #36611: [SPARK-39204][CORE] Change `Utils.createTempDir` and `Utils.createDirectory` call the same logic method in `JavaUtils`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36611:
URL: https://github.com/apache/spark/pull/36611#discussion_r877674754
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -308,28 +308,7 @@ private[spark] object Utils extends Logging {
* newly created, and is not marked for automatic deletion.
*/
def createDirectory(root: String, namePrefix: String = "spark"): File = {
- var attempts = 0
- val maxAttempts = MAX_DIR_CREATION_ATTEMPTS
- var dir: File = null
- while (dir == null) {
- attempts += 1
- if (attempts > maxAttempts) {
- throw new IOException("Failed to create a temp directory (under " + root + ") after " +
- maxAttempts + " attempts!")
- }
- try {
- dir = new File(root, namePrefix + "-" + UUID.randomUUID.toString)
- // SPARK-35907:
- // This could throw more meaningful exception information if directory creation failed.
- Files.createDirectories(dir.toPath)
- } catch {
- case e @ (_ : IOException | _ : SecurityException) =>
- logError(s"Failed to create directory $dir", e)
- dir = null
- }
- }
-
- dir.getCanonicalFile
+ JavaUtils.createDirectory(root, namePrefix)
Review Comment:
https://github.com/apache/spark/blob/5ee6f72744143cc5e19aa058df209f7156e51cee/common/network-common/src/main/java/org/apache/spark/network/util/JavaUtils.java#L399-L419
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #36611: [SPARK-39204][CORE] Change `Utils.createTempDir` and `Utils.createDirectory` call the same logic method in `JavaUtils`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #36611:
URL: https://github.com/apache/spark/pull/36611#issuecomment-1132373369
> Yeah, I think we should better fix `Utils.createTempDir`.
Yeah ~ now this pr only change one file and achieved the goal
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #36611: [SPARK-39204][CORE] Change `Utils.createTempDir` and `Utils.createDirectory` call the same logic method in `JavaUtils`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36611:
URL: https://github.com/apache/spark/pull/36611#discussion_r877674586
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -339,9 +318,7 @@ private[spark] object Utils extends Logging {
def createTempDir(
root: String = System.getProperty("java.io.tmpdir"),
namePrefix: String = "spark"): File = {
- val dir = createDirectory(root, namePrefix)
- ShutdownHookManager.registerShutdownDeleteDir(dir)
- dir
+ JavaUtils.createTempDir(root, namePrefix)
Review Comment:
https://github.com/apache/spark/blob/5ee6f72744143cc5e19aa058df209f7156e51cee/common/network-common/src/main/java/org/apache/spark/network/util/JavaUtils.java#L379-L385
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srowen commented on pull request #36611: [SPARK-39204][CORE] Change `Utils.createTempDir` and `Utils.createDirectory` call the same logic method in `JavaUtils`
Posted by GitBox <gi...@apache.org>.
srowen commented on PR #36611:
URL: https://github.com/apache/spark/pull/36611#issuecomment-1133644962
Merged to master
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #36611: [SPARK-39204][CORE] Change `Utils.createTempDir` and `Utils.createDirectory` call the same logic method in `JavaUtils`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #36611:
URL: https://github.com/apache/spark/pull/36611#discussion_r878325387
##########
core/src/main/scala/org/apache/spark/util/Utils.scala:
##########
@@ -308,28 +308,7 @@ private[spark] object Utils extends Logging {
* newly created, and is not marked for automatic deletion.
*/
def createDirectory(root: String, namePrefix: String = "spark"): File = {
- var attempts = 0
- val maxAttempts = MAX_DIR_CREATION_ATTEMPTS
- var dir: File = null
- while (dir == null) {
- attempts += 1
- if (attempts > maxAttempts) {
- throw new IOException("Failed to create a temp directory (under " + root + ") after " +
- maxAttempts + " attempts!")
- }
- try {
- dir = new File(root, namePrefix + "-" + UUID.randomUUID.toString)
- // SPARK-35907:
- // This could throw more meaningful exception information if directory creation failed.
- Files.createDirectories(dir.toPath)
- } catch {
- case e @ (_ : IOException | _ : SecurityException) =>
- logError(s"Failed to create directory $dir", e)
- dir = null
- }
- }
-
- dir.getCanonicalFile
+ JavaUtils.createDirectory(root, namePrefix)
Review Comment:
In order to solve
```
Avoid the use of Guava's Files.createTempDir() in Spark code due to [CVE-2020-8908](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-8908)
```
I added two compatible methods in `JavaUtils` ([SPARK-39102](https://issues.apache.org/jira/browse/SPARK-39102)).
> Hm, why did we not just implement the logic here? rather than use JavaUtils? I forget if there was a reason
Code in `network-common` and `network-shuffle` module cannot call `Utils.createDirectory ` or `Utils. createTempDir` in `core` module.
--
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org