You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/10/13 04:01:55 UTC
[GitHub] [spark] warrenzhu25 opened a new pull request, #38231: [SPARK-40778][CORE] Make HeartbeatReceiver as an IsolatedRpcEndpoint
warrenzhu25 opened a new pull request, #38231:
URL: https://github.com/apache/spark/pull/38231
### What changes were proposed in this pull request?
Make HeartbeatReceiver as an IsolatedRpcEndpoint then it has dedicated single thread to process heartbeats.
### Why are the changes needed?
All RpcEndpoint including HeartbeatReceiver in driver are sharing one thread pool. When there're lots of rpc messages queued, the waiting process time of heartbeat time could easily exceed heartbeat timeout, which generates lots of false positive.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Manually tested.
--
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
[GitHub] [spark] AmplabJenkins commented on pull request #38231: [SPARK-40778][CORE] Make HeartbeatReceiver as an IsolatedRpcEndpoint
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #38231:
URL: https://github.com/apache/spark/pull/38231#issuecomment-1278383103
Can one of the admins verify this patch?
--
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
[GitHub] [spark] dongjoon-hyun closed pull request #38231: [SPARK-40778][CORE] Make HeartbeatReceiver as an IsolatedRpcEndpoint
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun closed pull request #38231: [SPARK-40778][CORE] Make HeartbeatReceiver as an IsolatedRpcEndpoint
URL: https://github.com/apache/spark/pull/38231
--
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
[GitHub] [spark] warrenzhu25 commented on pull request #38231: [SPARK-40778][CORE] Make HeartbeatReceiver as an IsolatedRpcEndpoint
Posted by GitBox <gi...@apache.org>.
warrenzhu25 commented on PR #38231:
URL: https://github.com/apache/spark/pull/38231#issuecomment-1283256514
> Gentle ping, @warrenzhu25
Thanks for gentle ping. Updated
--
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
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38231: [SPARK-40778][CORE] Make HeartbeatReceiver as an IsolatedRpcEndpoint
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38231:
URL: https://github.com/apache/spark/pull/38231#discussion_r995933565
##########
core/src/main/scala/org/apache/spark/HeartbeatReceiver.scala:
##########
@@ -65,7 +65,7 @@ private[spark] case class HeartbeatResponse(reregisterBlockManager: Boolean)
* Lives in the driver to receive heartbeats from executors..
*/
private[spark] class HeartbeatReceiver(sc: SparkContext, clock: Clock)
- extends SparkListener with ThreadSafeRpcEndpoint with Logging {
+ extends SparkListener with ThreadSafeRpcEndpoint with IsolatedRpcEndpoint with Logging {
Review Comment:
Do we need both `ThreadSafeRpcEndpoint` or `IsolatedRpcEndpoint`?
--
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
[GitHub] [spark] mridulm commented on a diff in pull request #38231: [SPARK-40778][CORE] Make HeartbeatReceiver as an IsolatedRpcEndpoint
Posted by GitBox <gi...@apache.org>.
mridulm commented on code in PR #38231:
URL: https://github.com/apache/spark/pull/38231#discussion_r996682781
##########
core/src/main/scala/org/apache/spark/HeartbeatReceiver.scala:
##########
@@ -65,7 +65,7 @@ private[spark] case class HeartbeatResponse(reregisterBlockManager: Boolean)
* Lives in the driver to receive heartbeats from executors..
*/
private[spark] class HeartbeatReceiver(sc: SparkContext, clock: Clock)
- extends SparkListener with ThreadSafeRpcEndpoint with Logging {
+ extends SparkListener with ThreadSafeRpcEndpoint with IsolatedRpcEndpoint with Logging {
Review Comment:
Agree with @dongjoon-hyun - we dont need both.
Given number of threads == 1, this should functionally behave like it is thread safe though.
I think the change of replace `ThreadSafeRpcEndpoint` with `IsolatedRpcEndpoint` should be fine.
But would be great if you can take a look @Ngone51.
Also, +CC @JoshRosen
--
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
[GitHub] [spark] cloud-fan commented on pull request #38231: [SPARK-40778][CORE] Make HeartbeatReceiver as an IsolatedRpcEndpoint
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on PR #38231:
URL: https://github.com/apache/spark/pull/38231#issuecomment-1283346066
cc @jiangxb1987
--
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
[GitHub] [spark] dongjoon-hyun commented on pull request #38231: [SPARK-40778][CORE] Make HeartbeatReceiver as an IsolatedRpcEndpoint
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #38231:
URL: https://github.com/apache/spark/pull/38231#issuecomment-1283263549
cc @cloud-fan , too.
--
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
[GitHub] [spark] warrenzhu25 commented on a diff in pull request #38231: [SPARK-40778][CORE] Make HeartbeatReceiver as an IsolatedRpcEndpoint
Posted by GitBox <gi...@apache.org>.
warrenzhu25 commented on code in PR #38231:
URL: https://github.com/apache/spark/pull/38231#discussion_r996207271
##########
core/src/main/scala/org/apache/spark/HeartbeatReceiver.scala:
##########
@@ -65,7 +65,7 @@ private[spark] case class HeartbeatResponse(reregisterBlockManager: Boolean)
* Lives in the driver to receive heartbeats from executors..
*/
private[spark] class HeartbeatReceiver(sc: SparkContext, clock: Clock)
- extends SparkListener with ThreadSafeRpcEndpoint with Logging {
+ extends SparkListener with ThreadSafeRpcEndpoint with IsolatedRpcEndpoint with Logging {
Review Comment:
> Do we need both `ThreadSafeRpcEndpoint` or `IsolatedRpcEndpoint`?
In theory, we only needs `IsolatedRpcEndpoint` as the default thread is 1, but I'm not sure whether there're some logic depending on trait `ThreadSafeRpcEndpoint`. To keep it safe, I just added new trait
--
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
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38231: [SPARK-40778][CORE] Make HeartbeatReceiver as an IsolatedRpcEndpoint
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38231:
URL: https://github.com/apache/spark/pull/38231#discussion_r996872791
##########
core/src/main/scala/org/apache/spark/HeartbeatReceiver.scala:
##########
@@ -65,7 +65,7 @@ private[spark] case class HeartbeatResponse(reregisterBlockManager: Boolean)
* Lives in the driver to receive heartbeats from executors..
*/
private[spark] class HeartbeatReceiver(sc: SparkContext, clock: Clock)
- extends SparkListener with ThreadSafeRpcEndpoint with Logging {
+ extends SparkListener with ThreadSafeRpcEndpoint with IsolatedRpcEndpoint with Logging {
Review Comment:
+1 for @mridulm 's comment.
--
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
[GitHub] [spark] warrenzhu25 commented on a diff in pull request #38231: [SPARK-40778][CORE] Make HeartbeatReceiver as an IsolatedRpcEndpoint
Posted by GitBox <gi...@apache.org>.
warrenzhu25 commented on code in PR #38231:
URL: https://github.com/apache/spark/pull/38231#discussion_r996207733
##########
core/src/main/scala/org/apache/spark/HeartbeatReceiver.scala:
##########
@@ -65,7 +65,7 @@ private[spark] case class HeartbeatResponse(reregisterBlockManager: Boolean)
* Lives in the driver to receive heartbeats from executors..
*/
private[spark] class HeartbeatReceiver(sc: SparkContext, clock: Clock)
- extends SparkListener with ThreadSafeRpcEndpoint with Logging {
+ extends SparkListener with ThreadSafeRpcEndpoint with IsolatedRpcEndpoint with Logging {
Review Comment:
> Could you elaborate your scale of testing? Are you using this in the production?
App with about 500+ executors. Yes, it's for production.
--
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
[GitHub] [spark] dongjoon-hyun commented on pull request #38231: [SPARK-40778][CORE] Make HeartbeatReceiver as an IsolatedRpcEndpoint
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #38231:
URL: https://github.com/apache/spark/pull/38231#issuecomment-1285234791
Thank you, @warrenzhu25 , @mridulm , @cloud-fan .
Merged to master for Apache Spark 3.4.
--
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
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38231: [SPARK-40778][CORE] Make HeartbeatReceiver as an IsolatedRpcEndpoint
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38231:
URL: https://github.com/apache/spark/pull/38231#discussion_r996612621
##########
core/src/main/scala/org/apache/spark/HeartbeatReceiver.scala:
##########
@@ -65,7 +65,7 @@ private[spark] case class HeartbeatResponse(reregisterBlockManager: Boolean)
* Lives in the driver to receive heartbeats from executors..
*/
private[spark] class HeartbeatReceiver(sc: SparkContext, clock: Clock)
- extends SparkListener with ThreadSafeRpcEndpoint with Logging {
+ extends SparkListener with ThreadSafeRpcEndpoint with IsolatedRpcEndpoint with Logging {
Review Comment:
I understand what you mean, but it's still not-safe completely, isn't it? If you are considering two different logic branches based on `ThreadSafeRpcEndpoint` or `IsolatedRpcEndpoint`, this PR could break some of the existing logic if the branching logic starts to match `IsolatedRpcEndpoint` first.
> I'm not sure whether there're some logic depending on trait ThreadSafeRpcEndpoint. To keep it safe, I just added new trait
--
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
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #38231: [SPARK-40778][CORE] Make HeartbeatReceiver as an IsolatedRpcEndpoint
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on code in PR #38231:
URL: https://github.com/apache/spark/pull/38231#discussion_r995930597
##########
core/src/main/scala/org/apache/spark/HeartbeatReceiver.scala:
##########
@@ -65,7 +65,7 @@ private[spark] case class HeartbeatResponse(reregisterBlockManager: Boolean)
* Lives in the driver to receive heartbeats from executors..
*/
private[spark] class HeartbeatReceiver(sc: SparkContext, clock: Clock)
- extends SparkListener with ThreadSafeRpcEndpoint with Logging {
+ extends SparkListener with ThreadSafeRpcEndpoint with IsolatedRpcEndpoint with Logging {
Review Comment:
Could you elaborate your scale of testing? Are you using this in the production?
--
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
[GitHub] [spark] dongjoon-hyun commented on pull request #38231: [SPARK-40778][CORE] Make HeartbeatReceiver as an IsolatedRpcEndpoint
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on PR #38231:
URL: https://github.com/apache/spark/pull/38231#issuecomment-1283248511
Gentle ping, @warrenzhu25
--
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