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

    https://github.com/apache/spark/pull/23083#discussion_r235955118
  
    --- Diff: 
core/src/main/scala/org/apache/spark/util/collection/ExternalSorter.scala ---
    @@ -727,9 +727,10 @@ private[spark] class ExternalSorter[K, V, C](
         spills.clear()
         forceSpillFiles.foreach(s => s.file.delete())
         forceSpillFiles.clear()
    -    if (map != null || buffer != null) {
    +    if (map != null || buffer != null || readingIterator != null) {
           map = null // So that the memory can be garbage-collected
           buffer = null // So that the memory can be garbage-collected
    +      readingIterator = null // So that the memory can be garbage-collected
    --- End diff --
    
    Hi @szhem , I discussed with wenchen offline. I think this is the key 
point. After nulling out `readingIterator`, `ExternalSorter` should released 
all the memories it occupied.
    
    Yes, `ExternalSorter` is leaked in `TaskCompletionListener`,  but it would 
already be stopped in `CompletionIterator` in happy path. The stopped sorter 
wouldn't occupy too much memory. The `readingIterator` 
    is occupying memory because it may reference 
`map/buffer.partitionedDestructiveSortedIterator`, which itself references 
`map/buffer`. So only nulling out map or buffer is not enough.
    
    Can you try with this modification only and see whether OOM still occurs.


---

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

Reply via email to