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

    https://github.com/apache/spark/pull/11628#discussion_r56133901
  
    --- Diff: core/src/main/scala/org/apache/spark/rdd/PipedRDD.scala ---
    @@ -176,6 +197,8 @@ private[spark] class PipedRDD[T: ClassTag](
     
               false
             }
    +        propagateChildThreadException()
    --- End diff --
    
    Yeah this is looking better. Should this occur at the start of the method?
    
    Should this method also perform cleanup? it seems like it doesn't happen if 
an exception is thrown while reading the input stream.
    
    I suppose it also doesn't happen if you call `next()` without `hasNext()`, 
although you do get `NoSuchElementException` now I believe, and this is a rare 
case. It doesn't seem like a significant problem.
    
    Also what happens if an error occurs reading the stderr stream?
    
    I know this is all not what you're trying to address here. I'm trying to 
also figure out how much to fix along the way here and how much to make a 
separate issue. 



---
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