[SPARK-13441] [YARN] Fix NPE in yarn Client.createConfArchive method

[chtyim]

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

[srowen]

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.

[chtyim]

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()

[srowen]

Yeah fair point, it has already been checked with isDirectory. OK this looks good to me.

[srowen]

Nit: this could be s"Failed to list files under directory $dir" but I don't think it's worth changing

[chtyim]

Thanks @srowen for the review. Do I need to do anything to get this PR merge?

[jerryshao]

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.

[chtyim]

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.

[srowen]

Jenkins, test this please

[SparkQA]

Test build #51966 has finished for PR 11337 at commit a8fb7cb.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[srowen]

Merged to master/1.6