[SPARK-9844][CORE] File appender race condition during shutdown

[BryanCutler]

When an Executor process is destroyed, the FileAppender that is asynchronously reading the stderr stream of the process can throw an IOException during read because the stream is closed. Before the ExecutorRunner destroys the process, the FileAppender thread is flagged to stop. This PR wraps the inputStream.read call of the FileAppender in a try/catch block so that if an IOException is thrown and the thread has been flagged to stop, it will safely ignore the exception. Additionally, the FileAppender thread was changed to use Utils.tryWithSafeFinally to better log any exception that do occur. Added unit tests to verify a IOException is thrown and logged if FileAppender is not flagged to stop, and that no IOException when the flag is set.

[SparkQA]

Test build #49198 has finished for PR 10714 at commit dc65737.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #49219 has finished for PR 10714 at commit 20d8e7b.

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

[srowen]

I don't think it's worth changing as this is fine, but I think you could just write

case _: IOException if markedForStop => // do nothing because ...

... but maybe I'm missing something about how that works.

[BryanCutler]

Thanks @srowen , I think your suggestion makes it easier to follow what is going on, so I made that change. I also changed the check for appending for only if positive bytes are read, which will just save a call to write if n is 0.

[SparkQA]

Test build #49256 has finished for PR 10714 at commit 4684a58.

• This patch fails PySpark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[BryanCutler]

jenkins retest this please

[SparkQA]

Test build #49263 has finished for PR 10714 at commit 4684a58.

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

[srowen]

LGTM

[srowen]

Merged to master, 1.6

[BryanCutler]

Thanks!

[yhuai]

@BryanCutler can you take a look at the failed test (https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-branch-1.6-test-maven-with-yarn-2.3/52/testReport/junit/org.apache.spark.util/FileAppenderSuite/file_appender_async_close_stream_abruptly/)?

[srowen]

@yhuai it's also possibly my cherry-pick to 1.6 was somehow not correct:
0c67993

If it's failing consistently let's just revert it for 1.6. If it's not consistent, give Bryan a chance to look first I guess.

[yhuai]

yea no problem. Will keep an eye on the test.

[BryanCutler]

Ah, I know what's going on @yhuai and @srowen . The cherry pick is fine, but I had SPARK-12701 merged earlier because of this reason, and it just went into master. Since the thread isn't properly joined, the test can exit before the log is written, and that is what the test is checking. Can we get SPARK-12701 also on 1.6?

[yhuai]

@srowen @zsxwing Is it safe to merge https://issues.apache.org/jira/browse/SPARK-12701 to branch 1.6? Or, we should revert it from branch 1.6?

[srowen]

I think it is safe to back-port. I'll do that now.

[BryanCutler]

Yeah, it should be safe to backport, only the tests actually call awaitTermination for this thread.