Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/11436#discussion_r54639039
--- Diff: core/src/main/scala/org/apache/spark/storage/BlockManager.scala
---
@@ -707,26 +719,33 @@ private[spark] class BlockManager(
tellMaster: Boolean = true,
effectiveStorageLevel: Option[StorageLevel] = None): Boolean = {
require(bytes != null, "Bytes is null")
- doPut(blockId, ByteBufferValues(bytes), level, tellMaster,
effectiveStorageLevel)
+ doPut(blockId, ByteBufferValues(bytes), level, tellMaster,
effectiveStorageLevel).isEmpty
}
/**
* Put the given block according to the given level in one of the block
stores, replicating
* the values if necessary.
*
- * The effective storage level refers to the level according to which
the block will actually be
- * handled. This allows the caller to specify an alternate behavior of
doPut while preserving
- * the original level specified by the user.
- *
- * @return true if the block was stored or false if the block was
already stored or an
- * error occurred.
+ * @param effectiveStorageLevel the level according to which the block
will actually be handled.
+ * This allows the caller to specify an
alternate behavior of doPut
+ * while preserving the original level
specified by the user.
+ * @param downgradeToReadLock if true, this method will downgrade its
write lock on the block
+ * to a read lock before returning; this will
happen even if the block
+ * already exists, so when this is true the
caller will always hold
+ * a read lock on the block after this method
returns without throwing
+ * an exception. If false (default), this
method will release the write
+ * lock before returning.
+ * @return `Some(PutResult)` if the block did not exist and could not be
successfully cached,
+ * or None if the block already existed or was successfully
stored (fully consuming
+ * the input data / input iterator).
--- End diff --
I read this many times and still found this confusing. Normally I would
expect to get `Some`thing when the operation succeeded, and `None` otherwise,
but this is the other way round. I think it would be simpler if we add a field
in `PutResult` that says `success: Boolean` or something and just always return
a `PutResult`. Then you don't need to get the block again after putting in
`getOrElseUpdate`, which I also found kind of unintuitive.
---
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]
