You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by kevincox <gi...@git.apache.org> on 2016/01/15 02:15:09 UTC
[GitHub] spark pull request: Refactor ExecutorAllocationManager.
GitHub user kevincox opened a pull request:
https://github.com/apache/spark/pull/10761
Refactor ExecutorAllocationManager.
This changes ExecuorAllocationManager from a tree of if
statements run on an interval to a clean event-driven state machine.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kevincox/spark refactor-executorallocationmanager
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/10761.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #10761
----
commit 115b9ca0e240dc17cc22ae31d8935e355013337e
Author: Kevin Cox <ke...@kevincox.ca>
Date: 2015-11-22T15:41:41Z
Refactor ExecutorAllocationManager.
This changes ExecuorAllocationManager from a tree of if
statements run on an interval to a clean event-driven state machine.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-15567] Refactor ExecutorAllocationManag...
Posted by kevincox <gi...@git.apache.org>.
Github user kevincox commented on the pull request:
https://github.com/apache/spark/pull/10761#issuecomment-221933006
Can this be reopened? I now have time to work on it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Refactor ExecutorAllocationManager.
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/10761#issuecomment-171851470
OK that makes sense. Thanks for explaining. The biggest problem with refactoring like this is you'd need to find somebody to review it, and it is pretty hard to review ...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Refactor ExecutorAllocationManager.
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the pull request:
https://github.com/apache/spark/pull/10761#issuecomment-213151310
@kevincox right.
- Usually, I believe such changes need a JIRA which manages the issues in Spark. Usually there sould be a JIRA to discuss a proper way to fix and problems **before** submitting a PR.
- This seems not following Spark guide lines for contribution, https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark
- It seems not following coding style guide lines, https://github.com/databricks/scala-style-guide.
- Recently, there was a discussion that there are currently too many PRs open which makes comitters (or "maintainers") hard to review. Please refer this link, http://apache-spark-developers-list.1001551.n3.nabble.com/auto-closing-pull-requests-that-have-been-inactive-gt-30-days-td17208.html
- Most importantly comitters think this should be closed for now. If contributors want committers respect contributers, then I think contributors respect them first.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Refactor ExecutorAllocationManager.
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the pull request:
https://github.com/apache/spark/pull/10761#issuecomment-212692762
ping @kevincox
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Refactor ExecutorAllocationManager.
Posted by kevincox <gi...@git.apache.org>.
Github user kevincox commented on the pull request:
https://github.com/apache/spark/pull/10761#issuecomment-172707827
It is similar but it has a number of advantages including that it can be cleaner (you can wait until after you finish a task and prepare it for shuffling) and you get more control as it can be configured on a per-job level without creating massive amounts of yarn queues.
Either way I would argue that this refactor is still warranted because of the cleaner and more efficient code but am definitely open to a discussion.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Refactor ExecutorAllocationManager.
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/10761#issuecomment-171835570
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Refactor ExecutorAllocationManager.
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/10761#issuecomment-171844919
What future features are you thinking about? Refactoring like this for the sake of refactoring is pretty dangerous ...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Refactor ExecutorAllocationManager.
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/10761#issuecomment-202642465
Let's close this PR until we have a proper design discussion on a JIRA.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Refactor ExecutorAllocationManager.
Posted by kevincox <gi...@git.apache.org>.
Github user kevincox commented on the pull request:
https://github.com/apache/spark/pull/10761#issuecomment-171835801
Note that this refactoring was performed to make way for future features in the allocation manager.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Refactor ExecutorAllocationManager.
Posted by kevincox <gi...@git.apache.org>.
Github user kevincox commented on the pull request:
https://github.com/apache/spark/pull/10761#issuecomment-213100491
@HyukjinKwon The maintainers didn't seem interested and I was busy. I might pursue this further this summer.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Refactor ExecutorAllocationManager.
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/10761
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Refactor ExecutorAllocationManager.
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/10761#issuecomment-213102473
This should be closed in the meantime
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Refactor ExecutorAllocationManager.
Posted by kevincox <gi...@git.apache.org>.
Github user kevincox commented on the pull request:
https://github.com/apache/spark/pull/10761#issuecomment-171851889
Fair enough. I tried to keep the tests as similar as possible so that it is quite clear that the functionality hasn't changed. I have also done some testing on my own and will be putting the change into production shortly to get some real-world banging on the changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Refactor ExecutorAllocationManager.
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the pull request:
https://github.com/apache/spark/pull/10761#issuecomment-213164527
@kevincox Could you please close this for now? You can easily reopen this once you start to work.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Refactor ExecutorAllocationManager.
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on the pull request:
https://github.com/apache/spark/pull/10761#issuecomment-172700451
Hi @kevincox , IIUC looks like your description of dynamic allocation is quite similar to some kinds of preemption mechanism in the cluster manager.
>This allows jobs to utilize an entire cluster when it is unneeded but when another job starts (especially development or interactive jobs) the currently running jobs can scale back to allow it in. This means that there is no longer a balance between cluster utilization and interactive job launching.
I'm doubting is it good to address such kind of resource related problem in application level? Since Spark is just an application that doesn't have a whole picture of cluster usage, to address such problem is quite hard for Spark. But for YARN, with capacity scheduler preemption enabled, also with priority supported this problem can be handled relatively easy.
The complexity of this module makes the refactor not an easy work, we might have a better discussion before refactoring on this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Refactor ExecutorAllocationManager.
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/10761#issuecomment-178224421
@kevincox please follow https://cwiki.apache.org/confluence/display/SPARK/Contributing+to+Spark. This task needs a JIRA and you need to link it in the title. For big features like this you need a design doc to accompany the issue.
However, it seems to me what you want is cooperative preemption. I believe the bulk of the required changes will lie in the interaction between the Spark driver and the cluster manager, not in `ExecutorAllocationManager`, so I believe refactoring this class at this scale doesn't actually help.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: Refactor ExecutorAllocationManager.
Posted by kevincox <gi...@git.apache.org>.
Github user kevincox commented on the pull request:
https://github.com/apache/spark/pull/10761#issuecomment-171850023
I'm implementing a system where Spark can reduce the number of executors in low-resource situations. This allows jobs to utilize an entire cluster when it is unneeded but when another job starts (especially development or interactive jobs) the currently running jobs can scale back to allow it in. This means that there is no longer a balance between cluster utilization and interactive job launching.
Before this refactor the changes for that feature were messy and disorganized, refactoring the class allows a simple implementation that requires a single new message and a single new state.
Also there are immediate benefits of the new design, the 100ms polling is gone for an event driven approach which will likely wake up every minute or so. Also cleaner code encourages future improvements :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org