Skip to content

[SPARK-5788] [PySpark] capture the exception in python write thread #4577

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
davies wants to merge 2 commits into from
Closed

[SPARK-5788] [PySpark] capture the exception in python write thread #4577

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
139b0db
capture the exception in python write thread
Feb 12, 2015
eb0ceff
Update PythonRDD.scala
Feb 17, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/src/main/scala/org/apache/spark/api/python/PythonRDD.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ private[spark] class PythonRDD(
this.interrupt()
}

override def run(): Unit = Utils.logUncaughtExceptions {
override def run(): Unit = Utils.tryLog {
Copy link
Contributor

@rxin rxin Feb 12, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing a big tryLog (which is sort of dangerous because of the potential of users adding a return statement that would break this code), can we add an explicit try around the two worker.shutdownOutput() calls?

Copy link
Contributor Author

@davies davies Feb 12, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The run can not have return value. The InterruptedException could happen in any place (when the task is canceled), so I think it's better to do it here.

Copy link
Contributor

@rxin rxin Feb 12, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can still call return even if the value is Unit

Copy link
Contributor

@rxin rxin Feb 12, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw this happened a few times in the past in Spark. somebody added a return in a giant block that is wrapped in a closure. Code reviewers couldn't catch it because it didn't show up in the diff.

Copy link
Contributor

@JoshRosen JoshRosen Feb 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rxin, is the concern that a user might write something like

def foo(): Unit = Utils.tryLog {
   return someCallThatThrowsAnException()
}

and thereby skip the exception-handling logic?

try {
val stream = new BufferedOutputStream(worker.getOutputStream, bufferSize)
val dataOut = new DataOutputStream(stream)
Expand Down