-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-15951] Change Executors Page to use datatables to support sorting columns and searching #13670
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
|
Can you post a screenshot of how it looks after the change? I'll take a look through the code when I have a chance. Also @zhuoliu want to take a look since you were the one who added JQuery DataTables to the History Server |
|
@ajbozarth , Thank you for loooking into this patch. I have added screenshot with annotation. I hope that helps. |
|
Jenkins, ok to test |
|
Test build #60584 has finished for PR 13670 at commit
|
|
Jenkins, test this please |
|
Test build #60601 has finished for PR 13670 at commit
|
|
Jenkins, test this please |
|
Test build #60612 has finished for PR 13670 at commit
|
|
Generally looks great to me, I am good with it as long as @tgravescs verifies. One minor concern is that we may move the utility functions from executorspage.js and historypage.ps to a common location. |
|
I agree with @zhuoliu we should move common/util functions into a datatables util js. For the above error, I pinpointed that it is baseURI that is null. I also never see the "Loading Executors Page" message i see in the code. I have also checked that the page doesn't load in any browser. @zhuoliu have you tried checking out and running the code? |
56f3901 to
3b4962a
Compare
|
Test build #61070 has finished for PR 13670 at commit
|
|
Test build #61387 has started for PR 13670 at commit |
|
@zhuoliu , i have removed unwanted, unused javascript functions in the executorspage.js. There are no duplicate functions now between historyserver.js and this script.. |
|
@ajbozarth, I do not see how I could produce the exception you see. Please help me reproduce this so that I can address it. |
docs/monitoring.md
Outdated
| <td>A list of all active executors for the given application.</td> | ||
| </tr> | ||
| <tr> | ||
| <td><code>/applications/[app-id]/executors</code></td> |
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.
These are duplicate end points: /applications/[app-id]/executors
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.
yep, I think you meant allexecutors
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.
fixed
|
I'll re-checkout later today and see if I can reproduce the error for you |
|
I get the error by just starting up a spark-shell, but I only get it on Safari, it works fine on Chrome and Firefox. I'll make a line note of what I see going wrong |
|
|
||
| function createRESTEndPoint() { | ||
| var parser = document.createElement('a'); | ||
| var words = parser.baseURI.split('/'); |
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.
In Safari baseURI is null, it seems to be how Safari handles document.createElement, you may want to research it and possibly find another way to get the URL, there are no other uses of baseURI in the Spark code base.
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.
As mentioned, I switched it to use document.baseURI which works with Safari as well.
|
@tgravescs , moved |
|
Test build #62343 has finished for PR 13670 at commit
|
|
Test build #62346 has finished for PR 13670 at commit
|
|
@ajbozarth were you able to try the latest version? |
| }); | ||
|
|
||
| executorsSummary = $("#active-executors"); | ||
| searchString = executorsSummary["context"]["location"]["search"]; |
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.
unused, remove
|
Been busy the last couple days, I'll take a look at this Monday |
|
@tgravescs, I removed unwanted code/comment from the javascript. |
|
Test build #62472 has finished for PR 13670 at commit
|
|
Sorry I couldn't get to this today, I'll do my best to take a look in the morning |
| return activeTasks > 0 ? ("hsla(240, 100%, 50%, " + activeTasksAlpha(activeTasks, maxTasks) + ")") : ""; | ||
| } | ||
|
|
||
| function activeTasksColor(activeTasks, maxTasks) { |
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.
This is never used
|
I checked out out the latest code and ran it and it looks good, I also looked through the code and outside of the two unused functions I found the code looks good too. Im comfortable giving this a LGTM We may want to start setting a standard way to convert pages to use DataTables, this seemed more a difficult process than I thought it would be |
|
@ajbozarth Thank you for review. I removed unused methods from the the javascript. I agree converting pages to |
|
Test build #62607 has finished for PR 13670 at commit
|
|
+1, thanks @kishorvpatil @ajbozarth |
|
Hi, @kishorvpatil. Cool improvement! I'm developing the Executors Page at #14204 based on your changes. But I have a small question about your new implementation of the tables. |
|
@nblintao , I am not sure why we had the check of existence of logs - specially when the reference to logs is static link for |
|
@kishorvpatil I can't believe I missed that. The logs only exist if you have logging turned on in your confs, if it's off there won't be any logs and no reason for the column. @nblintao if you want to file a JIRA either you @kishorvpatil or I can quickly fix that |
|
personally I don't see this as a big deal. As long as it doesn't show the links, the column being there doesn't hurt anything and it keeps things more consistent. Perhaps change it to say Not Available or something? If you thinkts its an issue lets file a separate jira for this and discuss further there. |
|
I opened SPARK-16673 for the issue since it also apply to the Thread Dump column which the History Server shouldn't display. |
…and Thread Dump columns ## What changes were proposed in this pull request? When #13670 switched `ExecutorsPage` to use JQuery DataTables it incidentally removed the conditional for the Logs and Thread Dump columns. I reimplemented the conditional display of the Logs and Thread dump columns as it was before the switch. ## How was this patch tested? Manually tested and dev/run-tests    Author: Alex Bozarth <[email protected]> Closes #14382 from ajbozarth/spark16673.
|
@kishorvpatil For the document We had better document more clearly what is meaning of functions for the 2.1 version. This confused people, because our test already run into this issue. |

Snapshots for how it looks like now:

New Executors Page screenshot looks like this:
