Github user srowen commented on a diff in the pull request:

    https://github.com/apache/spark/pull/12194#discussion_r64680641
  
    --- Diff: 
sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/ScriptTransformation.scala
 ---
    @@ -127,45 +127,78 @@ case class ScriptTransformation(
             }
             val mutableRow = new SpecificMutableRow(output.map(_.dataType))
     
    +        private def checkFailureAndPropagate(cause: Throwable = null): 
Unit = {
    +          if (writerThread.exception.isDefined) {
    +            throw writerThread.exception.get
    +          }
    +
    +          // Checks if the proc is still alive (incase the command ran was 
bad)
    +          // The ideal way to do this is to use Java 8's Process#isAlive()
    +          // but it cannot be used because Spark still supports Java 7.
    +          // Following is a workaround used to check if a process is alive 
in Java 7
    +          // TODO: Once builds are switched to Java 8, this can be changed
    +          try {
    +            val exitCode = proc.exitValue()
    +            if (exitCode != 0) {
    +              logError(stderrBuffer.toString) // log the stderr circular 
buffer
    +              throw new SparkException(s"Subprocess exited with status 
$exitCode. " +
    +                s"Error: ${stderrBuffer.toString}", cause)
    +            }
    +          } catch {
    +            case _: IllegalThreadStateException =>
    +            // This means that the process is still alive. Move ahead
    +          }
    +        }
    +
             override def hasNext: Boolean = {
    -          if (outputSerde == null) {
    -            if (curLine == null) {
    -              curLine = reader.readLine()
    +          try {
    +            if (outputSerde == null) {
                   if (curLine == null) {
    -                if (writerThread.exception.isDefined) {
    -                  throw writerThread.exception.get
    +                curLine = reader.readLine()
    +                if (curLine == null) {
    +                  checkFailureAndPropagate()
    +                  false
    +                } else {
    +                  true
    --- End diff --
    
    Hm, is it me or does this get hard to follow the return values. Generally 
the method returns "true" unless one of several conditions caused it to decide 
it was finished earlier. Those could be handled with early "return false" 
rather than lots of "else ... true" branches.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

Reply via email to