Github user JoshRosen commented on the issue:
https://github.com/apache/spark/pull/21342
Thanks for the updates. The net change / scope of changes have been
significantly reduced here, so I feel that this change is a lot less risky now.
I left only one nitpicky comment at
https://github.com/apache/spark/pull/21342/files#r189753488 worrying about
potential future risks from people coming along and writing new code throwing
`SparkFatalException` in a context where it can bubble up to the uncaught
exception handler. If we want to be super defensive we could add some logic at
https://github.com/apache/spark/blob/32447079e9d0fa9f7e180b94ecac19091b6af1ab/core/src/main/scala/org/apache/spark/util/SparkUncaughtExceptionHandler.scala#L42
to also catch a `SparkFatalException` on top of an OOM and treat that as an
OOM. Debatable if we want to do that, but it's a great way of addressing
@gatorsmile's comment at
https://github.com/apache/spark/pull/21342#discussion_r189627101 and avoids
future breakage.
Otherwise, LGTM.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
