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

    https://github.com/apache/spark/pull/17295#discussion_r108046686
  
    --- Diff: core/src/main/scala/org/apache/spark/storage/BlockManager.scala 
---
    @@ -56,6 +57,49 @@ private[spark] class BlockResult(
         val bytes: Long)
     
     /**
    + * Abstracts away how blocks are stored and provides different ways to 
read the underlying block
    + * data. Callers should call [[dispose()]] when they're done with the 
block.
    + */
    +private[spark] trait BlockData {
    +
    +  def toInputStream(): InputStream
    +
    +  /**
    +   * Returns a Netty-friendly wrapper for the block's data.
    +   *
    +   * @see [[ManagedBuffer#convertToNetty()]]
    +   */
    +  def toNetty(): Object
    +
    +  def toChunkedByteBuffer(allocator: Int => ByteBuffer): ChunkedByteBuffer
    +
    +  def toByteBuffer(): ByteBuffer
    +
    +  def size: Long
    +
    +  def dispose(): Unit
    +
    +}
    +
    +private[spark] class ByteBufferBlockData(val buffer: ChunkedByteBuffer) 
extends BlockData {
    +
    +  override def toInputStream(): InputStream = buffer.toInputStream(dispose 
= false)
    +
    +  override def toNetty(): Object = buffer.toNetty
    +
    +  override def toChunkedByteBuffer(allocator: Int => ByteBuffer): 
ChunkedByteBuffer = {
    +    buffer.copy(allocator)
    +  }
    +
    +  override def toByteBuffer(): ByteBuffer = buffer.toByteBuffer
    +
    +  override def size: Long = buffer.size
    +
    +  override def dispose(): Unit = buffer.unmap()
    --- End diff --
    
    I think `BlockData.dispose()` is pretty well defined. "Release any 
resources held by the object." What's confusing is that there's both 
`dispose()` and `unmap()` in `ChunkedByteBuffer`, when there used to be only 
`dispose()`. It's confusing to have two different methods for releasing 
resources, and that confusion is not being caused by this patch.
    
    `BlockData` is not just a wrapper around `ChunkedByteBuffer`; if it were 
there wouldn't be a need for it. Which is why calling the method `unmmap()` 
wouldn't make any sense here, since that's very specific to memory-mapped byte 
buffers.


---
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 [email protected] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to