[SPARK-9516][UI] Improvement of Thread Dump Page #7910

CodingCat · 2015-08-03T22:45:46Z

https://issues.apache.org/jira/browse/SPARK-9516

new look of Thread Dump Page
click column title to sort
grep
search as you type

@squito @JoshRosen It's ready for the review now

CodingCat · 2015-08-03T22:47:57Z

CodingCat · 2015-08-03T22:48:39Z

SparkQA · 2015-08-03T23:08:33Z

Test build #39606 has finished for PR 7910 at commit 635e696.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2015-08-04T05:20:59Z

Test build #39641 has finished for PR 7910 at commit 416aece.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

squito · 2015-08-04T17:35:26Z

Hi @CodingCat just a quick question from looking at the screenshot -- can you still click on one thread to expand the stack trace for just that? nothing looks like a hyperlink anymore. Again I'm not a UI expert by any means, so I'll defer to others opinions, but I wouldn't think they were clickable from the look of it.

SparkQA · 2015-08-04T18:00:30Z

Test build #39721 has finished for PR 7910 at commit a62ee4b.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2015-08-04T19:37:02Z

Test build #39719 has finished for PR 7910 at commit 4d1211e.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

CodingCat · 2015-08-05T01:42:28Z

Hi, @squito , sorry for the late reply, here is the screenshot

the rows are still clickable...I will add onmouseover event tomorrow to make it more obvious...

SparkQA · 2015-08-05T04:20:19Z

Test build #39799 has finished for PR 7910 at commit 5666ad8.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

CodingCat · 2015-08-05T15:26:52Z

added mouseover event

SparkQA · 2015-08-05T19:48:40Z

Test build #39874 timed out for PR 7910 at commit 101e9a9 after a configured wait of 175m.

SparkQA · 2015-08-05T23:16:31Z

Test build #39914 has finished for PR 7910 at commit 263d835.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2015-08-06T02:14:41Z

Test build #39944 has finished for PR 7910 at commit 8995d0c.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

squito · 2015-08-06T16:40:47Z

Jenkins, retest this please

squito · 2015-08-06T16:59:45Z

core/src/main/scala/org/apache/spark/ui/exec/ExecutorThreadDumpPage.scala

+        if (!grepExp.isDefined) {
+          true
+        } else {
+          thread.stackTrace.filter(_ >= ' ').matches(grepExp.get)


The filter seems strange to me. Won't stripping out newlines etc. lead to some weird matches?

Also, I think String.matches may not be what we want -- that matches against the entire string, not against any contained substring. It would most likely mean that everyone would have to make their pattern ".*<pattern>.*". I think we'd rather use pattern.find(), since that is more like "grep". Should probably also be in multiline mode, so:

val grepExp = Option(request.getParamter("grepexp")).map(Pattern.compile(_, Pattern.MULTILINE)) ... grepExp.map { grep => grep.matcher(thread.stackTrace).find()}.getOrElse(true)

SparkQA · 2015-08-06T17:06:31Z

Test build #40043 has finished for PR 7910 at commit 8995d0c.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

CodingCat · 2015-08-06T18:21:43Z

Grep with ForkJoinPool

CodingCat · 2015-08-06T18:23:25Z

Thanks, @squito , I addressed your comments and uploaded the latest version of code

SparkQA · 2015-08-06T19:00:23Z

Test build #255 has finished for PR 7910 at commit 8995d0c.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2015-08-06T20:32:54Z

Test build #40065 has finished for PR 7910 at commit 1e43da0.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

CodingCat · 2015-08-06T20:42:45Z

-_-|||

SparkQA · 2015-08-06T23:07:02Z

Test build #40073 has finished for PR 7910 at commit 837ed6e.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class IsotonicRegression(override val uid: String) extends Estimator[IsotonicRegressionModel]

CodingCat · 2015-08-07T00:18:38Z

it seems that Jenkins is very very unstable in these days

SparkQA · 2015-08-07T03:15:50Z

Test build #40112 has finished for PR 7910 at commit d49e094.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

JoshRosen · 2015-08-07T05:08:56Z

Yeah, we've been combatting a really bad test flakiness issue over the past 48 hours :(

JoshRosen · 2015-10-16T20:04:14Z

I just tested this out and noticed a few problems:

The full-text search doesn't seem to properly search all columns; I tried some queries and found that it returned fewer rows than expected.
The hover styling on the tables causes the width to jump around when the text becomes bold. I'd prefer to only highlight the background of the table cells.
It would be nice if the cursor changed to indicate that table cells are clickable; this should be fixable with CSS.
The new layout makes the thread dumps harder to read. Rather than adding an additional column when a thread-dump is displayed, I think it would make more sense to expand the table row so that the thread dump appears underneath the table columns in that row. This isn't trivial to implement, but I still think we should do it.

Given all of this, I don't think that this PR is ready to merge in its current form. @CodingCat, if you don't anticipate having time to make these changes, would you mind closing this in the meantime to help de-clutter the PR review queue? Thanks!

CodingCat · 2015-10-17T00:01:41Z

I might work on this during the week and next week, so, I prefer to keeping this open

SparkQA · 2015-10-22T16:53:16Z

Test build #44152 has finished for PR 7910 at commit c3dc9b7.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

CodingCat · 2015-10-22T21:15:24Z

CodingCat · 2015-10-22T21:19:50Z

@JoshRosen @squito

I adjusted the patch according to Josh's suggestions,

now the search is covering all columns (I thought it only cares about stacktrace column)

the stacktrace is shown as a new row

For the StackTrace row, the current implementation is to add a new row following the row showing Thread ID, Name and State, instead of toggling on "hidden" class

the reason is that the hidden stacktrace row is a big trouble when you sort and then expand, or grep some lines.....

so....Welcome to review it.....

SparkQA · 2015-10-22T23:25:38Z

Test build #44175 has finished for PR 7910 at commit e88f70d.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

CodingCat · 2015-10-30T11:56:15Z

@squito @JoshRosen just ping...

squito · 2015-10-30T15:22:18Z

core/src/main/resources/org/apache/spark/ui/static/table.js

+}
+
+function onSearchStringChange() {
+    var searchString = $('#search').val()


add a toLowerCase here -- I tried searching for "RUNNABLE" first and it didn't find anything.

squito · 2015-10-30T15:55:49Z

Hi @CodingCat, thanks for the reminder. This is looking great! I really like it. Just found a few minor things, and one design question about highlighting the task threads.

CodingCat · 2015-11-01T23:27:14Z

@squito Thanks for the comments, just addressed the issues

SparkQA · 2015-11-02T01:57:07Z

Test build #44780 has finished for PR 7910 at commit 8bfb4bc.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

squito · 2015-11-02T19:37:26Z

core/src/main/resources/org/apache/spark/ui/static/table.js

+function onSearchStringChange() {
+    var searchString = $('#search').val().toLowerCase();
+    //remove the stacktrace
+    collapseAllThreadStackTrace(false, false)


this only takes 1 arg now (crazy that this works in js ...)

oops, my bad

squito · 2015-11-02T19:38:13Z

thanks for the update @CodingCat , just one tiny nit, otherwise lgtm. @JoshRosen would you like to take another look?

SparkQA · 2015-11-03T03:44:16Z

Test build #44861 has finished for PR 7910 at commit 6106c7f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

CodingCat · 2015-11-09T01:25:23Z

Hi, @JoshRosen , since 1.6 has been cut, will we push this feature in the coming version?

andrewor14 · 2015-12-15T01:19:13Z

retest this please

andrewor14 · 2015-12-15T01:19:59Z

Looks great! I'm going to merge this in master if no one objects.

SparkQA · 2015-12-15T03:26:45Z

Test build #47699 has finished for PR 7910 at commit 6106c7f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

andrewor14 · 2015-12-16T02:21:19Z

Merged into master. Thanks @CodingCat and everyone who reviewed this!

CodingCat · 2015-12-16T21:05:12Z

thanks @JoshRosen and @squito for reviewing it

thanks for @andrewor14 for merging

CodingCat force-pushed the SPARK-9516 branch from 635e696 to 416aece Compare August 4, 2015 02:39

squito reviewed Aug 6, 2015
View reviewed changes

CodingCat force-pushed the SPARK-9516 branch from 1e43da0 to 837ed6e Compare August 6, 2015 20:43

CodingCat force-pushed the SPARK-9516 branch from 837ed6e to d49e094 Compare August 7, 2015 00:18

CodingCat force-pushed the SPARK-9516 branch from d49e094 to 33bb3e3 Compare August 7, 2015 12:26

sort alphabetically and change the css class name

4843c40

search over all columns and highlight background

c3dc9b7

adjust the page structure,

e88f70d

squito reviewed Oct 30, 2015
View reviewed changes

separate expand and collapse function

8bfb4bc

squito reviewed Nov 2, 2015
View reviewed changes

collapseAllThreadStackTrace takes only one parameter

6106c7f

asfgit closed this in a63d9ed Dec 16, 2015

CodingCat deleted the SPARK-9516 branch December 16, 2015 21:04

[SPARK-9516][UI] Improvement of Thread Dump Page #7910

[SPARK-9516][UI] Improvement of Thread Dump Page #7910

Conversation

CodingCat commented Aug 3, 2015

CodingCat commented Aug 3, 2015

CodingCat commented Aug 3, 2015

SparkQA commented Aug 3, 2015

SparkQA commented Aug 4, 2015

squito commented Aug 4, 2015

SparkQA commented Aug 4, 2015

SparkQA commented Aug 4, 2015

CodingCat commented Aug 5, 2015

SparkQA commented Aug 5, 2015

CodingCat commented Aug 5, 2015

SparkQA commented Aug 5, 2015

SparkQA commented Aug 5, 2015

SparkQA commented Aug 6, 2015

squito commented Aug 6, 2015

squito Aug 6, 2015

Choose a reason for hiding this comment

SparkQA commented Aug 6, 2015

CodingCat commented Aug 6, 2015

CodingCat commented Aug 6, 2015

SparkQA commented Aug 6, 2015

SparkQA commented Aug 6, 2015

CodingCat commented Aug 6, 2015

SparkQA commented Aug 6, 2015

CodingCat commented Aug 7, 2015

SparkQA commented Aug 7, 2015

JoshRosen commented Aug 7, 2015

JoshRosen commented Oct 16, 2015

CodingCat commented Oct 17, 2015

SparkQA commented Oct 22, 2015

CodingCat commented Oct 22, 2015

CodingCat commented Oct 22, 2015

SparkQA commented Oct 22, 2015

CodingCat commented Oct 30, 2015

squito Oct 30, 2015

Choose a reason for hiding this comment

squito commented Oct 30, 2015

CodingCat commented Nov 1, 2015

SparkQA commented Nov 2, 2015

squito Nov 2, 2015

Choose a reason for hiding this comment

CodingCat Nov 3, 2015

Choose a reason for hiding this comment

squito commented Nov 2, 2015

SparkQA commented Nov 3, 2015

CodingCat commented Nov 9, 2015

andrewor14 commented Dec 15, 2015

andrewor14 commented Dec 15, 2015

SparkQA commented Dec 15, 2015

andrewor14 commented Dec 16, 2015

CodingCat commented Dec 16, 2015