You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by chtyim <gi...@git.apache.org> on 2016/02/24 01:34:00 UTC
[GitHub] spark pull request: [SPARK-13441] [YARN] Fix NPE in yarn Client.cr...
GitHub user chtyim opened a pull request:
https://github.com/apache/spark/pull/11337
[SPARK-13441] [YARN] Fix NPE in yarn Client.createConfArchive method
## What changes were proposed in this pull request?
Instead of using result of File.listFiles() directly, which may throw NPE, check for null first. If it is null, log a warning instead
## How was the this patch tested?
Ran the ./dev/run-tests locally
Tested manually on a cluster
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/chtyim/spark fixes/SPARK-13441-null-check
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/11337.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 #11337
----
commit a8fb7cb0be0588d4c31a1fc24c3473ef39562be5
Author: Terence Yim <te...@cask.co>
Date: 2016-02-24T00:27:48Z
[SPARK-13441] [YARN] Fix NPE in yarn Client.createConfArchive method
- Log a warning instead of throwing NPE if either
$HADOOP_CONF_DIR or $YARN_CONF_DIR is not accessible.
----
---
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: [SPARK-13441] [YARN] Fix NPE in yarn Client.cr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11337#issuecomment-187988518
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: [SPARK-13441] [YARN] Fix NPE in yarn Client.cr...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/11337#discussion_r53923847
--- Diff: yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala ---
@@ -537,9 +537,14 @@ private[spark] class Client(
sys.env.get(envKey).foreach { path =>
val dir = new File(path)
if (dir.isDirectory()) {
- dir.listFiles().foreach { file =>
- if (file.isFile && !hadoopConfFiles.contains(file.getName())) {
- hadoopConfFiles(file.getName()) = file
+ val files = dir.listFiles()
+ if (files == null) {
+ logWarning("Failed to list files under directory " + dir)
--- End diff --
I don't think that means it (necessarily) failed; it can return null if it's empty. I think a simple != null check is all you want 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: [SPARK-13441] [YARN] Fix NPE in yarn Client.cr...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on the pull request:
https://github.com/apache/spark/pull/11337#issuecomment-188416520
Thanks @srowen for the review. Do I need to do anything to get this PR merge?
---
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: [SPARK-13441] [YARN] Fix NPE in yarn Client.cr...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on a diff in the pull request:
https://github.com/apache/spark/pull/11337#discussion_r53968653
--- Diff: yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala ---
@@ -537,9 +537,14 @@ private[spark] class Client(
sys.env.get(envKey).foreach { path =>
val dir = new File(path)
if (dir.isDirectory()) {
- dir.listFiles().foreach { file =>
- if (file.isFile && !hadoopConfFiles.contains(file.getName())) {
- hadoopConfFiles(file.getName()) = file
+ val files = dir.listFiles()
+ if (files == null) {
+ logWarning("Failed to list files under directory " + dir)
--- End diff --
According to the Java API doc, it returns empty array if the directory is empty. It only returns null if it is not a directory or have IO error (permission issue). Without logging a warning, we might be silently ignoring misconfiguration.
Ref: https://docs.oracle.com/javase/7/docs/api/java/io/File.html#listFiles()
---
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: [SPARK-13441] [YARN] Fix NPE in yarn Client.cr...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/11337
---
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: [SPARK-13441] [YARN] Fix NPE in yarn Client.cr...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on the pull request:
https://github.com/apache/spark/pull/11337#issuecomment-188671358
Is it better to throw an exception and fail fast? I'm wondering the behavior of application is still correct if we miss these Hadoop related configurations.
---
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: [SPARK-13441] [YARN] Fix NPE in yarn Client.cr...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/11337#issuecomment-188785924
Merged to master/1.6
---
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: [SPARK-13441] [YARN] Fix NPE in yarn Client.cr...
Posted by chtyim <gi...@git.apache.org>.
Github user chtyim commented on the pull request:
https://github.com/apache/spark/pull/11337#issuecomment-188707421
The reason why failing is bad in this case is because if either the `HADOOP_CONF_DIR` or the `YARN_CONF_DIR` is not readable, exception is raised, even if one of those directories already provides all the hadoop conf files that are needed by Spark.
E.g. in one of our use case, we actually launch Spark job from a YARN container, which the `HADOOP_CONF_DIR` is set to be a private directory that is owned by the NM (that's how Cloudera CM works anyway), which the container fails to read. Even we try to workaround it by setting the `YARN_CONF_DIR` to a readable directory, however, due to the exception being raised, we can't submit the job even those hadoop files are available.
---
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: [SPARK-13441] [YARN] Fix NPE in yarn Client.cr...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/11337#discussion_r53970359
--- Diff: yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala ---
@@ -537,9 +537,14 @@ private[spark] class Client(
sys.env.get(envKey).foreach { path =>
val dir = new File(path)
if (dir.isDirectory()) {
- dir.listFiles().foreach { file =>
- if (file.isFile && !hadoopConfFiles.contains(file.getName())) {
- hadoopConfFiles(file.getName()) = file
+ val files = dir.listFiles()
+ if (files == null) {
+ logWarning("Failed to list files under directory " + dir)
--- End diff --
Yeah fair point, it has already been checked with `isDirectory`. OK this looks good to me.
---
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: [SPARK-13441] [YARN] Fix NPE in yarn Client.cr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11337#issuecomment-188765548
**[Test build #51966 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/51966/consoleFull)** for PR 11337 at commit [`a8fb7cb`](https://github.com/apache/spark/commit/a8fb7cb0be0588d4c31a1fc24c3473ef39562be5).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
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: [SPARK-13441] [YARN] Fix NPE in yarn Client.cr...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11337#issuecomment-188757862
**[Test build #51966 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/51966/consoleFull)** for PR 11337 at commit [`a8fb7cb`](https://github.com/apache/spark/commit/a8fb7cb0be0588d4c31a1fc24c3473ef39562be5).
---
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: [SPARK-13441] [YARN] Fix NPE in yarn Client.cr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11337#issuecomment-188765890
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/51966/
Test PASSed.
---
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: [SPARK-13441] [YARN] Fix NPE in yarn Client.cr...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11337#issuecomment-188765886
Merged build finished. Test PASSed.
---
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: [SPARK-13441] [YARN] Fix NPE in yarn Client.cr...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/11337#issuecomment-188757345
Jenkins, test this please
---
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: [SPARK-13441] [YARN] Fix NPE in yarn Client.cr...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/11337#discussion_r54086677
--- Diff: yarn/src/main/scala/org/apache/spark/deploy/yarn/Client.scala ---
@@ -537,9 +537,14 @@ private[spark] class Client(
sys.env.get(envKey).foreach { path =>
val dir = new File(path)
if (dir.isDirectory()) {
- dir.listFiles().foreach { file =>
- if (file.isFile && !hadoopConfFiles.contains(file.getName())) {
- hadoopConfFiles(file.getName()) = file
+ val files = dir.listFiles()
+ if (files == null) {
+ logWarning("Failed to list files under directory " + dir)
--- End diff --
Nit: this could be `s"Failed to list files under directory $dir"` but I don't think it's worth changing
---
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