You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "LuciferYang (via GitHub)" <gi...@apache.org> on 2024/03/20 06:00:41 UTC
[PR] [SPARK-47474][CORE] Revert change of SPARK-47461 and add some comments [spark]
LuciferYang opened a new pull request, #45602:
URL: https://github.com/apache/spark/pull/45602
### What changes were proposed in this pull request?
This pr revert the change of SPARK-47461 and add some comments to `ExecutorAllocationManager#totalRunningTasksPerResourceProfile` to clarify that the tests in `ExecutorAllocationManagerSuite` need to call `listener.totalRunningTasksPerResourceProfile` with `synchronized`.
### Why are the changes needed?
`ExecutorAllocationManagerSuite` need to call `listener.totalRunningTasksPerResourceProfile` with `synchronized`.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Pass GitHub Actions
### 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-47474][CORE] Revert SPARK-47461 and add some comments [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45602:
URL: https://github.com/apache/spark/pull/45602#issuecomment-2011281562
Thank you, @LuciferYang and all!
--
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-47474][CORE] Revert SPARK-47461 and add some comments [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang closed pull request #45602: [SPARK-47474][CORE] Revert SPARK-47461 and add some comments
URL: https://github.com/apache/spark/pull/45602
--
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-47474][CORE] Revert change of SPARK-47461 and add some comments [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #45602:
URL: https://github.com/apache/spark/pull/45602#discussion_r1531542734
##########
core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala:
##########
@@ -320,6 +320,12 @@ private[spark] class ExecutorAllocationManager(
}
}
+ // SPARK-47474: Please do not delete this function, the tests in `ExecutorAllocationManagerSuite`
+ // need to access `listener.totalRunningTasksPerResourceProfile` with `synchronized`.
+ private def totalRunningTasksPerResourceProfile(id: Int): Int = synchronized {
Review Comment:
cc @dongjoon-hyun @mridulm @tgravescs
Since new comments were added during the revert, I have created a new Jira. Is there a better way to describe the content of the comments?
--
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-47474][CORE] Revert SPARK-47461 and add some comments [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #45602:
URL: https://github.com/apache/spark/pull/45602#issuecomment-2011052480
> BTW, @LuciferYang , as you requested in the above already, I guess we need to wait for @mridulm and @tgravescs 's review and approval. I'll leave this to them.
OK
--
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-47474][CORE] Revert change of SPARK-47461 and add some comments [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #45602:
URL: https://github.com/apache/spark/pull/45602#discussion_r1532155113
##########
core/src/main/scala/org/apache/spark/ExecutorAllocationManager.scala:
##########
@@ -320,6 +320,12 @@ private[spark] class ExecutorAllocationManager(
}
}
+ // SPARK-47474: Please do not delete this function, the tests in `ExecutorAllocationManagerSuite`
Review Comment:
Let's remove `SPARK-47474: ` here because the comment looks sufficient.
--
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-47474][CORE] Revert SPARK-47461 and add some comments [spark]
Posted by "tgravescs (via GitHub)" <gi...@apache.org>.
tgravescs commented on PR #45602:
URL: https://github.com/apache/spark/pull/45602#issuecomment-2009828525
+1
--
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-47474][CORE] Revert SPARK-47461 and add some comments [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45602:
URL: https://github.com/apache/spark/pull/45602#issuecomment-2009767644
BTW, @LuciferYang , as you requested in the above already, I guess we need to wait for @mridulm and @tgravescs 's review and approval. I'll leave this to them.
--
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-47474][CORE] Revert SPARK-47461 and add some comments [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #45602:
URL: https://github.com/apache/spark/pull/45602#issuecomment-2011280003
Merged into master ~ thanks @dongjoon-hyun @tgravescs @mridulm
--
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