-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-7007][core] Add a metric source for ExecutorAllocationManager #5589
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Test build #30580 has finished for PR 5589 at commit
|
|
Jenkins, retest this please. |
|
Test build #30587 has finished for PR 5589 at commit
|
|
Test build #30999 has finished for PR 5589 at commit
|
|
@jerryshao this looks good. It will allow us to plot these variables in very interesting ways. Note that this relies on internal variables that are subject to change. For instance, #5536 gets rid of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind putting this as an inner class at the bottom (like ExecutorAllocationManagerListener) instead?
|
Also it seems that you need to rebase this one. |
|
Hi @andrewor14 , thanks a lot for your comments, I will change the code accordingly. |
a6d5ec5 to
08ff59e
Compare
|
Test build #31218 has finished for PR 5589 at commit
|
|
Hi @andrewor14 , I've changed the code according to your comments, please take a look at this, thanks a lot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class is already private[spark], so we don't need to declare this again here.
|
Test build #31364 has finished for PR 5589 at commit
|
|
@jerryshao I believe this doesn't compile anymore since I merged #5074. Would you mind rebasing this? |
|
@jerryshao no specific concerns about the implementation. If @andrewor14 is ok with the inconsistency, I'm fine with it too. |
|
Yeah it's actually going to be a little tricky to bring this outside because this accesses a whole bunch of internal variables. I even think that we should add a big comment at the top of the source class to say this is experimental and there are no backward compatibility guarantees at all. |
6cb0c37 to
104d155
Compare
|
@andrewor14 @sryza , thanks a lot for your comments, I just rebase the codes, please review it. |
|
Test build #31656 has finished for PR 5589 at commit
|
|
Looks great. Merging into master and 1.4 thanks @sryza @jerryshao |
Add a metric source to expose the internal status of ExecutorAllocationManager to better monitoring the resource usage of executors when dynamic allocation is enable. Please help to review, thanks a lot. Author: jerryshao <[email protected]> Closes #5589 from jerryshao/dynamic-allocation-source and squashes the following commits: 104d155 [jerryshao] rebase and address the comments c501a2c [jerryshao] Address the comments d237ba5 [jerryshao] Address the comments 2c3540f [jerryshao] Add a metric source for ExecutorAllocationManager (cherry picked from commit 9f1f9b1) Signed-off-by: Andrew Or <[email protected]>
Add a metric source to expose the internal status of ExecutorAllocationManager to better monitoring the resource usage of executors when dynamic allocation is enable. Please help to review, thanks a lot. Author: jerryshao <[email protected]> Closes apache#5589 from jerryshao/dynamic-allocation-source and squashes the following commits: 104d155 [jerryshao] rebase and address the comments c501a2c [jerryshao] Address the comments d237ba5 [jerryshao] Address the comments 2c3540f [jerryshao] Add a metric source for ExecutorAllocationManager
Add a metric source to expose the internal status of ExecutorAllocationManager to better monitoring the resource usage of executors when dynamic allocation is enable. Please help to review, thanks a lot. Author: jerryshao <[email protected]> Closes apache#5589 from jerryshao/dynamic-allocation-source and squashes the following commits: 104d155 [jerryshao] rebase and address the comments c501a2c [jerryshao] Address the comments d237ba5 [jerryshao] Address the comments 2c3540f [jerryshao] Add a metric source for ExecutorAllocationManager
Add a metric source to expose the internal status of ExecutorAllocationManager to better monitoring the resource usage of executors when dynamic allocation is enable. Please help to review, thanks a lot. Author: jerryshao <[email protected]> Closes apache#5589 from jerryshao/dynamic-allocation-source and squashes the following commits: 104d155 [jerryshao] rebase and address the comments c501a2c [jerryshao] Address the comments d237ba5 [jerryshao] Address the comments 2c3540f [jerryshao] Add a metric source for ExecutorAllocationManager
Add a metric source to expose the internal status of ExecutorAllocationManager to better monitoring the resource usage of executors when dynamic allocation is enable. Please help to review, thanks a lot.