You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by liu-zhaokun <gi...@git.apache.org> on 2017/08/04 01:48:16 UTC
[GitHub] spark pull request #18838: [SPARK-21632] There is no need to make attempts f...
GitHub user liu-zhaokun opened a pull request:
https://github.com/apache/spark/pull/18838
[SPARK-21632] There is no need to make attempts for createDirectory if the dir had existed
[https://issues.apache.org/jira/browse/SPARK-21632](https://issues.apache.org/jira/browse/SPARK-21632)
There is no need to make attempts for createDirectory if the dir had existed.So I think we should log it,and Jump out of the loop
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/liu-zhaokun/spark master08040833
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/18838.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #18838
----
commit e3e36a1b920890f2e1873790775d4327cf9ee4fa
Author: liuzhaokun <li...@zte.com.cn>
Date: 2017-08-04T01:39:51Z
[SPARK-21632] There is no need to make attempts for createDirectory if the dir had existed
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18838: [SPARK-21632] There is no need to make attempts for crea...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on the issue:
https://github.com/apache/spark/pull/18838
I think the original purpose is to void reusing the directory and always create a unique directory. So with the change here seems the semantics is changed, I don't think it is a reasonable fix here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18838: [SPARK-21632] There is no need to make attempts f...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/18838#discussion_r131326239
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -294,9 +294,14 @@ private[spark] object Utils extends Logging {
}
try {
dir = new File(root, namePrefix + "-" + UUID.randomUUID.toString)
- if (dir.exists() || !dir.mkdirs()) {
+ if (!dir.mkdirs()) {
dir = null
}
+ if (dir.exists()) {
--- End diff --
I guess it throws NPE if we failed to create a dir?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18838: [SPARK-21632] There is no need to make attempts for crea...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/spark/pull/18838
@jerryshao
Thanks for your reply.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18838: [SPARK-21632] There is no need to make attempts for crea...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/18838
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18838: [SPARK-21632] There is no need to make attempts f...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/18838#discussion_r131326197
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -294,9 +294,14 @@ private[spark] object Utils extends Logging {
}
try {
dir = new File(root, namePrefix + "-" + UUID.randomUUID.toString)
- if (dir.exists() || !dir.mkdirs()) {
--- End diff --
I think we intendedly try to return a non-existent and a newly created directory. It looks not a hot path and performance here looks not a big deal.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18838: [SPARK-21632] There is no need to make attempts for crea...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/18838
This introduces an NPE. It also changes what happens when the dir exists. I don't see a problem that this solves, so I'd close this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18838: [SPARK-21632] There is no need to make attempts for crea...
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on the issue:
https://github.com/apache/spark/pull/18838
Let's close this PR, since the original behavior was by design.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18838: [SPARK-21632] There is no need to make attempts for crea...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/18838
Could we close this one?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18838: [SPARK-21632] There is no need to make attempts for crea...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun commented on the issue:
https://github.com/apache/spark/pull/18838
@srowen
Yes,I change the logic because I think it's no mean to attempt to create a dir while there was a same name dir.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #18838: [SPARK-21632] There is no need to make attempts for crea...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/18838
This changes the logic and I'm not clear why it's an improvement.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18838: [SPARK-21632] There is no need to make attempts f...
Posted by liu-zhaokun <gi...@git.apache.org>.
Github user liu-zhaokun closed the pull request at:
https://github.com/apache/spark/pull/18838
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #18838: [SPARK-21632] There is no need to make attempts f...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/18838#discussion_r131328303
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -294,9 +294,14 @@ private[spark] object Utils extends Logging {
}
try {
dir = new File(root, namePrefix + "-" + UUID.randomUUID.toString)
- if (dir.exists() || !dir.mkdirs()) {
+ if (!dir.mkdirs()) {
dir = null
}
+ if (dir.exists()) {
+ logInfo(s"$dir exists,can't create a new directory.")
+ dir = null
+ return dir.getCanonicalFile
--- End diff --
And here it looks always NPE.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org