You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "EnricoMi (via GitHub)" <gi...@apache.org> on 2026/02/09 10:33:12 UTC

[PR] [SPARK-55447][CORE] Fix spark.cleaner.referenceTracking.blocking.timeout config [spark]

EnricoMi opened a new pull request, #54224:
URL: https://github.com/apache/spark/pull/54224

   ### What changes were proposed in this pull request?
   Makes `removeRdd`, `removeShuffle` and `removeBroadcast` truly non-blocking when `blocking == false`. Also fixes actual timeout of the future.
   
   ### Why are the changes needed?
   - Calling `driverEndpoint.askSync(message)` creates a future with `timeout = RpcUtils.askRpcTimeout(conf)`: This ignores `waitBlockRemovalTimeout`.
   - Then `askSync` (as the name implies) waits for the result of the future: `timeout.awaitResult(future)`. This ignores `blocking == false`.
   
   ### Does this PR introduce _any_ user-facing change?
   No.
   
   ### How was this patch tested?
   Only through code review.
   
   ### Was this patch authored or co-authored using generative AI tooling?
   No


-- 
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

Re: [PR] [SPARK-55447][CORE] Fix spark.cleaner.referenceTracking.blocking.timeout config [spark]

Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #54224:
URL: https://github.com/apache/spark/pull/54224#discussion_r2793972813


##########
core/src/main/scala/org/apache/spark/storage/BlockManagerMaster.scala:
##########
@@ -193,40 +193,48 @@ class BlockManagerMaster(
 
   /** Remove all blocks belonging to the given RDD. */
   def removeRdd(rddId: Int, blocking: Boolean): Unit = {
-    val future = driverEndpoint.askSync[Future[Seq[Int]]](RemoveRdd(rddId))
+    val msg = RemoveRdd(rddId)
+    val future = if (blocking) {
+      driverEndpoint.askSync[Future[Seq[Int]]](msg, waitBlockRemovalTimeout)
+    } else {
+      driverEndpoint.ask[Future[Seq[Int]]](msg)
+    }

Review Comment:
   Ahh, `askSync` is a synchronous transfer of the `Future`, while the future most likely is not completed. And `askSync(..., timeout)` is the timeout of the sync transfer. Thanks for clarification!



-- 
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

Re: [PR] [SPARK-55447][CORE] Fix spark.cleaner.referenceTracking.blocking.timeout config [spark]

Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi closed pull request #54224: [SPARK-55447][CORE] Fix spark.cleaner.referenceTracking.blocking.timeout config
URL: https://github.com/apache/spark/pull/54224


-- 
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

Re: [PR] [SPARK-55447][CORE] Fix spark.cleaner.referenceTracking.blocking.timeout config [spark]

Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on code in PR #54224:
URL: https://github.com/apache/spark/pull/54224#discussion_r2793972813


##########
core/src/main/scala/org/apache/spark/storage/BlockManagerMaster.scala:
##########
@@ -193,40 +193,48 @@ class BlockManagerMaster(
 
   /** Remove all blocks belonging to the given RDD. */
   def removeRdd(rddId: Int, blocking: Boolean): Unit = {
-    val future = driverEndpoint.askSync[Future[Seq[Int]]](RemoveRdd(rddId))
+    val msg = RemoveRdd(rddId)
+    val future = if (blocking) {
+      driverEndpoint.askSync[Future[Seq[Int]]](msg, waitBlockRemovalTimeout)
+    } else {
+      driverEndpoint.ask[Future[Seq[Int]]](msg)
+    }

Review Comment:
   Ahh, `askSync` is a synchronous transfer of the `Future`, while the future most likely is not completed.



-- 
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

Re: [PR] [SPARK-55447][CORE] Fix spark.cleaner.referenceTracking.blocking.timeout config [spark]

Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on PR #54224:
URL: https://github.com/apache/spark/pull/54224#issuecomment-3870935727

   @AngersZhuuuu @dongjoon-hyun @Ngone51 The timeout introduced in #52919 is eclipsed by `RpcUtils.askRpcTimeout(conf)`. And `blocking = true` is still blocking. Details in the PR description.


-- 
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

Re: [PR] [SPARK-55447][CORE] Fix spark.cleaner.referenceTracking.blocking.timeout config [spark]

Posted by "Ngone51 (via GitHub)" <gi...@apache.org>.
Ngone51 commented on code in PR #54224:
URL: https://github.com/apache/spark/pull/54224#discussion_r2791076859


##########
core/src/main/scala/org/apache/spark/storage/BlockManagerMaster.scala:
##########
@@ -193,40 +193,48 @@ class BlockManagerMaster(
 
   /** Remove all blocks belonging to the given RDD. */
   def removeRdd(rddId: Int, blocking: Boolean): Unit = {
-    val future = driverEndpoint.askSync[Future[Seq[Int]]](RemoveRdd(rddId))
+    val msg = RemoveRdd(rddId)
+    val future = if (blocking) {
+      driverEndpoint.askSync[Future[Seq[Int]]](msg, waitBlockRemovalTimeout)
+    } else {
+      driverEndpoint.ask[Future[Seq[Int]]](msg)
+    }

Review Comment:
   I think `blocking` controls whether we should wait for the returned `Future` to complete rather than whether wait for the RPC response (the PRC reponse is expected to be fast here, though I know there could also be abnormal cases that timed-out in the end). So I think the original behaviour of `blocking` should be correct, unless you want to add new blocking flag for the RPC response.



-- 
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

Re: [PR] [SPARK-55447][CORE] Fix spark.cleaner.referenceTracking.blocking.timeout config [spark]

Posted by "EnricoMi (via GitHub)" <gi...@apache.org>.
EnricoMi commented on PR #54224:
URL: https://github.com/apache/spark/pull/54224#issuecomment-3870951343

   Ideas on how to unit test this welcome. Ideally, I would assert the returned future's timeout and whether `awaitResult` has been / has not been called.


-- 
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