You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "JoshRosen (via GitHub)" <gi...@apache.org> on 2023/09/26 16:55:37 UTC

[GitHub] [spark] JoshRosen commented on a diff in pull request #43067: [SPARK-45057][CORE] Avoid acquire read lock when keepReadLock is false

JoshRosen commented on code in PR #43067:
URL: https://github.com/apache/spark/pull/43067#discussion_r1337527770


##########
core/src/main/scala/org/apache/spark/storage/BlockInfoManager.scala:
##########
@@ -421,7 +421,8 @@ private[storage] class BlockInfoManager(trackingCacheVisibility: Boolean = false
    */
   def lockNewBlockForWriting(

Review Comment:
   Minor doc nit:
   
   The `@return` docstring a couple of lines should probably be updated because the existing statement about read locks is inaccurate after your change.
   
   I think the new condition could be something like this
   
   > `@return` true if the block did not already exist, false otherwise. If this returns `true`, a write lock on the new block will be held. If this returns `false` then a read lock will be held if `keepReadLock == true`. 



##########
core/src/main/scala/org/apache/spark/storage/BlockInfoManager.scala:
##########
@@ -421,7 +421,8 @@ private[storage] class BlockInfoManager(trackingCacheVisibility: Boolean = false
    */
   def lockNewBlockForWriting(

Review Comment:
   Minor doc nit:
   
   The `@return` docstring a couple of lines should probably be updated because the existing statement about read locks is inaccurate after your change.
   
   I think the new condition could be something like this
   
   > `@return` true if the block did not already exist, false otherwise. If this returns `true`, a write lock on the new block will be held. If this returns `false` then a read lock will be held iff `keepReadLock == true`. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


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