[SPARK-35027][CORE] Close the inputStream in FileAppender when writin… #33263

jhu-chang · 2021-07-08T09:29:21Z

What changes were proposed in this pull request?

add "closeStreams" to FileAppender and RollingFileAppender
set "closeStreams" to "true" in ExecutorRunner

Why are the changes needed?

The executor will hang when due disk full or other exceptions which happened in writting to outputStream: the root cause is the "inputStream" is not closed after the error happens:

ExecutorRunner creates two files appenders for pipe: one for stdout, one for stderr
FileAppender.appendStreamToFile exits the loop when writing to outputStream
FileAppender closes the outputStream, but left the inputStream which refers the pipe's stdout and stderr opened
The executor will hang when printing the log message if the pipe is full (no one consume the outputs)
From the driver side, you can see the task can't be completed for ever

With this fix, the step 4 will throw an exception, the driver can catch up the exception and reschedule the failed task to other executors.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Add new tests for the "closeStreams" in FileAppenderSuite

…g the logs failure ### What changes were proposed in this pull request? 1. Add an option to close the input streams in FileAppender 1. Set the closeStreams to true in FileAppender when used in ExecutorRunner ### Why are the changes needed? The "inputStream" in FileAppender is not closed when error happened in writting to outputStream ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Add new tests for FileAppender

core/src/main/scala/org/apache/spark/util/logging/FileAppender.scala

srowen · 2021-07-09T18:20:57Z

Jenkins test this please

SparkQA · 2021-07-09T19:05:53Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45376/

SparkQA · 2021-07-09T19:39:44Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45376/

SparkQA · 2021-07-09T20:54:35Z

Test build #140865 has finished for PR 33263 at commit 8b2b540.

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

core/src/main/scala/org/apache/spark/deploy/worker/ExecutorRunner.scala

HyukjinKwon · 2021-07-16T02:38:06Z

core/src/main/scala/org/apache/spark/util/logging/FileAppender.scala

+  inputStream: InputStream,
+  file: File,
+  bufferSize: Int = 8192,
+  closeStreams: Boolean = false


shall we name it closeInputStream?

Look at this again. When will we explicitly set closeStreams to false? cc @jhu-chang

HyukjinKwon · 2021-07-16T02:38:39Z

core/src/test/scala/org/apache/spark/util/FileAppenderSuite.scala

@@ -61,6 +61,15 @@ class FileAppenderSuite extends SparkFunSuite with BeforeAndAfter with Logging {
    assert(Files.toString(testFile, StandardCharsets.UTF_8) === header + testString)
  }

+  test("basic file appender - close stream") {


Let's add a JIRA prefix here e.g.) "SPARK-35027: basic file ..."

HyukjinKwon · 2021-07-16T02:39:07Z

core/src/test/scala/org/apache/spark/util/FileAppenderSuite.scala

@@ -96,6 +105,32 @@ class FileAppenderSuite extends SparkFunSuite with BeforeAndAfter with Logging {
      appender, testOutputStream, textToAppend, rolloverIntervalMillis, isCompressed = true)
  }

+  test("rolling file appender - time-based rolling close stream") {


ditto for jira prefix

HyukjinKwon · 2021-07-16T02:39:26Z

core/src/test/scala/org/apache/spark/util/FileAppenderSuite.scala

+    verify(inputStream).close()
+  }
+
+  test("rolling file appender - size-based rolling close stream") {


HyukjinKwon

Looks fine otherwise. cc @Ngone51 FYI

core/src/main/scala/org/apache/spark/util/logging/FileAppender.scala

Ngone51 · 2021-07-20T12:12:25Z

@jhu-chang You probably need to enable Github Actions to trigger the test according to the tips here: https://github.com/apache/spark/pull/33263/checks?check_run_id=3111884720

srowen · 2021-07-20T12:56:40Z

Jenkins test this please

SparkQA · 2021-07-20T14:47:59Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45849/

SparkQA · 2021-07-20T15:24:51Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45849/

SparkQA · 2021-07-20T16:19:18Z

Test build #141333 has finished for PR 33263 at commit 955d246.

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

### What changes were proposed in this pull request? 1. add "closeStreams" to FileAppender and RollingFileAppender 2. set "closeStreams" to "true" in ExecutorRunner ### Why are the changes needed? The executor will hang when due disk full or other exceptions which happened in writting to outputStream: the root cause is the "inputStream" is not closed after the error happens: 1. ExecutorRunner creates two files appenders for pipe: one for stdout, one for stderr 2. FileAppender.appendStreamToFile exits the loop when writing to outputStream 3. FileAppender closes the outputStream, but left the inputStream which refers the pipe's stdout and stderr opened 4. The executor will hang when printing the log message if the pipe is full (no one consume the outputs) 5. From the driver side, you can see the task can't be completed for ever With this fix, the step 4 will throw an exception, the driver can catch up the exception and reschedule the failed task to other executors. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Add new tests for the "closeStreams" in FileAppenderSuite Closes #33263 from jhu-chang/SPARK-35027. Authored-by: Jie <gt.hu.chang@gmail.com> Signed-off-by: Sean Owen <srowen@gmail.com> (cherry picked from commit 1a8c675) Signed-off-by: Sean Owen <srowen@gmail.com>

srowen · 2021-07-21T02:25:27Z

Merged to master/3.2/3.1/3.0

### What changes were proposed in this pull request? 1. add "closeStreams" to FileAppender and RollingFileAppender 2. set "closeStreams" to "true" in ExecutorRunner ### Why are the changes needed? The executor will hang when due disk full or other exceptions which happened in writting to outputStream: the root cause is the "inputStream" is not closed after the error happens: 1. ExecutorRunner creates two files appenders for pipe: one for stdout, one for stderr 2. FileAppender.appendStreamToFile exits the loop when writing to outputStream 3. FileAppender closes the outputStream, but left the inputStream which refers the pipe's stdout and stderr opened 4. The executor will hang when printing the log message if the pipe is full (no one consume the outputs) 5. From the driver side, you can see the task can't be completed for ever With this fix, the step 4 will throw an exception, the driver can catch up the exception and reschedule the failed task to other executors. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Add new tests for the "closeStreams" in FileAppenderSuite Closes apache#33263 from jhu-chang/SPARK-35027. Authored-by: Jie <gt.hu.chang@gmail.com> Signed-off-by: Sean Owen <srowen@gmail.com> (cherry picked from commit 1a8c675) Signed-off-by: Sean Owen <srowen@gmail.com>

github-actions bot added the CORE label Jul 8, 2021

jhu-chang changed the title ~~[SPARK-35027][CORE] Close the inputStream in FileAppender when writin…~~ [WIP][SPARK-35027][CORE] Close the inputStream in FileAppender when writin… Jul 8, 2021

jhu-chang changed the title ~~[WIP][SPARK-35027][CORE] Close the inputStream in FileAppender when writin…~~ [SPARK-35027][CORE] Close the inputStream in FileAppender when writin… Jul 9, 2021

jhu-chang marked this pull request as ready for review July 9, 2021 05:21

srowen reviewed Jul 9, 2021

View reviewed changes

core/src/main/scala/org/apache/spark/util/logging/FileAppender.scala Show resolved Hide resolved

add optional "closeStreams" parameter rather override apply(...)

e4b90a0

HyukjinKwon reviewed Jul 12, 2021

View reviewed changes

core/src/main/scala/org/apache/spark/deploy/worker/ExecutorRunner.scala Show resolved Hide resolved

HyukjinKwon reviewed Jul 16, 2021

View reviewed changes

HyukjinKwon approved these changes Jul 16, 2021

View reviewed changes

Ngone51 reviewed Jul 16, 2021

View reviewed changes

core/src/main/scala/org/apache/spark/util/logging/FileAppender.scala Show resolved Hide resolved

add SPARK-35027 prefix to new added tests

955d246

Ngone51 approved these changes Jul 20, 2021

View reviewed changes

srowen closed this in 1a8c675 Jul 21, 2021

jhu-chang deleted the SPARK-35027 branch July 22, 2021 03:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-35027][CORE] Close the inputStream in FileAppender when writin… #33263

[SPARK-35027][CORE] Close the inputStream in FileAppender when writin… #33263

jhu-chang commented Jul 8, 2021 •

edited by Ngone51

srowen commented Jul 9, 2021

SparkQA commented Jul 9, 2021

SparkQA commented Jul 9, 2021

SparkQA commented Jul 9, 2021

HyukjinKwon Jul 16, 2021

Ngone51 Jul 28, 2021

HyukjinKwon Jul 16, 2021

HyukjinKwon Jul 16, 2021

HyukjinKwon Jul 16, 2021

HyukjinKwon left a comment

Ngone51 commented Jul 20, 2021

srowen commented Jul 20, 2021

SparkQA commented Jul 20, 2021

SparkQA commented Jul 20, 2021

SparkQA commented Jul 20, 2021

srowen commented Jul 21, 2021

[SPARK-35027][CORE] Close the inputStream in FileAppender when writin… #33263

[SPARK-35027][CORE] Close the inputStream in FileAppender when writin… #33263

Conversation

jhu-chang commented Jul 8, 2021 • edited by Ngone51

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

srowen commented Jul 9, 2021

SparkQA commented Jul 9, 2021

SparkQA commented Jul 9, 2021

SparkQA commented Jul 9, 2021

HyukjinKwon Jul 16, 2021

Choose a reason for hiding this comment

Ngone51 Jul 28, 2021

Choose a reason for hiding this comment

HyukjinKwon Jul 16, 2021

Choose a reason for hiding this comment

HyukjinKwon Jul 16, 2021

Choose a reason for hiding this comment

HyukjinKwon Jul 16, 2021

Choose a reason for hiding this comment

HyukjinKwon left a comment

Choose a reason for hiding this comment

Ngone51 commented Jul 20, 2021

srowen commented Jul 20, 2021

SparkQA commented Jul 20, 2021

SparkQA commented Jul 20, 2021

SparkQA commented Jul 20, 2021

srowen commented Jul 21, 2021

jhu-chang commented Jul 8, 2021 •

edited by Ngone51