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/19 09:23:16 UTC
[PR] [SPARK-47461][CORE] Remove unused private function `totalRunningTasksPerResourceProfile` from `ExecutorAllocationManager` [spark]
LuciferYang opened a new pull request, #45587:
URL: https://github.com/apache/spark/pull/45587
### What changes were proposed in this pull request?
This pr just remove unused private function `totalRunningTasksPerResourceProfile` from `ExecutorAllocationManager`
### Why are the changes needed?
Code clean up
### 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-47461][CORE] Remove private function `totalRunningTasksPerResourceProfile` from `ExecutorAllocationManager` [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #45587:
URL: https://github.com/apache/spark/pull/45587#issuecomment-2008713396
Submitted a PR for revert: 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-47461][CORE] Remove private function `totalRunningTasksPerResourceProfile` from `ExecutorAllocationManager` [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #45587:
URL: https://github.com/apache/spark/pull/45587#issuecomment-2008669180
I was looking at it from point of view of, is the change safe - is the behavior the same before and after ?
From that point of view, at face value of the PR, it appears that it need not be the same after the PR - the access was protected earlier by a synchronized lock, and could have ensured visibility of updates.
I have ofcourse not done an analysis of whether this safety is required, are there other mitigating reasons why it is still safe (perhaps an outer lock, etc) ... and so tagged Tom.
Note that there is also diminishing reasons given this is for tests, and is a private method :-)
--
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-47461][CORE] Remove private function `totalRunningTasksPerResourceProfile` from `ExecutorAllocationManager` [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #45587:
URL: https://github.com/apache/spark/pull/45587#issuecomment-2008672641
To give context, I am prototyping dynamically adapting a stage/tasks resource profile based on runtime execution behavior, and so happened to have looked at this code recently and had gone through a similar "is this method useless" question :-)
--
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-47461][CORE] Remove private function `totalRunningTasksPerResourceProfile` from `ExecutorAllocationManager` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45587:
URL: https://github.com/apache/spark/pull/45587#issuecomment-2007854448
Merged to master for Apache Spark 4.0.0. Thank you, @LuciferYang .
--
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-47461][CORE] Remove private function `totalRunningTasksPerResourceProfile` from `ExecutorAllocationManager` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45587:
URL: https://github.com/apache/spark/pull/45587#issuecomment-2008700496
+1 for (1) adding a new comment to clarify that this `synchronized` function needs to be retained for testing.
I'm not sure for (2) because it's a separate issue in this context.
--
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-47461][CORE] Remove private function `totalRunningTasksPerResourceProfile` from `ExecutorAllocationManager` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45587:
URL: https://github.com/apache/spark/pull/45587#issuecomment-2008097171
To @mridulm and @tgravescs , do you mean you are depending on the `private` method of `ExecutorAllocationManager` somehow? Just for my understanding, could you elaborate how you are affected?
```scala
private def totalRunningTasksPerResourceProfile(id: Int): Int = synchronized {
listener.totalRunningTasksPerResourceProfile(id)
}
```
--
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-47461][CORE] Remove private function `totalRunningTasksPerResourceProfile` from `ExecutorAllocationManager` [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #45587:
URL: https://github.com/apache/spark/pull/45587#issuecomment-2008697337
1. I am fine to revert this commit, but while revert, I think some comments should be added to the `totalRunningTasksPerResourceProfile ` function to clarify that this function needs to be retained for testing.
2. Another thing I want to clarify is, should we restrict the access level of listener to private? Would this action pose any risks?
@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
Re: [PR] [SPARK-47461][CORE] Remove private function `totalRunningTasksPerResourceProfile` from `ExecutorAllocationManager` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #45587: [SPARK-47461][CORE] Remove private function `totalRunningTasksPerResourceProfile` from `ExecutorAllocationManager`
URL: https://github.com/apache/spark/pull/45587
--
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-47461][CORE] Remove private function `totalRunningTasksPerResourceProfile` from `ExecutorAllocationManager` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45587:
URL: https://github.com/apache/spark/pull/45587#issuecomment-2008100572
Do you mean that we need to revert this commit because `ExecutorAllocationManagerSuite` needs that for stable test coverage?
--
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-47461][CORE] Remove private function `totalRunningTasksPerResourceProfile` from `ExecutorAllocationManager` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45587:
URL: https://github.com/apache/spark/pull/45587#issuecomment-2008694731
Thank you for the context, @mridulm and @tgravescs .
WDYT, @LuciferYang . Shall we revert this simply for now and revisit later (if needed), @LuciferYang ?
--
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-47461][CORE] Remove private function `totalRunningTasksPerResourceProfile` from `ExecutorAllocationManager` [spark]
Posted by "tgravescs (via GitHub)" <gi...@apache.org>.
tgravescs commented on PR #45587:
URL: https://github.com/apache/spark/pull/45587#issuecomment-2007935739
well it definitely changes the synchronization, everywhere else that is referenced is in synchronized block. Now since its only testing I'm not sure if you will see any issues there. At the same time this feels like one of those prs that doesn't need to be done and is just introducing risk.
It was also abstracting things so you weren't going straight into the listener. I think listener should really be private here but looking at history was opened up for a test that doesn't exist.
Personally I would prefer to see this reverted
--
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