Github user squito commented on a diff in the pull request:
https://github.com/apache/spark/pull/23058#discussion_r237268983
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManager.scala
---
@@ -789,21 +785,31 @@ private[spark] class BlockManager(
}
if (data != null) {
- // SPARK-24307 undocumented "escape-hatch" in case there are any
issues in converting to
- // ChunkedByteBuffer, to go back to old code-path. Can be removed
post Spark 2.4 if
- // new path is stable.
- if (remoteReadNioBufferConversion) {
- return Some(new ChunkedByteBuffer(data.nioByteBuffer()))
- } else {
- return Some(ChunkedByteBuffer.fromManagedBuffer(data))
- }
+ assert(!data.isInstanceOf[BlockManagerManagedBuffer])
--- End diff --
@wypoon sorry one more thing -- can you add a comment here explaining why
we have this assert? Otherwise it looks kinda random. Something like "The
lifecycyle of BlockManagerManagedBuffer is slightly different, in particular
wrt dispose of offheap buffers. Currently this path will never generate a
BlockManagerManagedBuffer (as we've just fetched the bytes remotely) -- this
assert is just to make sure that isn't changed in the future (or if it is, the
lifecycle is reconsidered)." I guess that's quite a long blurb, but think its
worthwhile.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
