[SPARK-16520] [WEBUI] Link executors to corresponding worker pages by nblintao · Pull Request #14204 · apache/spark

nblintao · 2016-07-14T15:58:26Z

What changes were proposed in this pull request?

Added a new column named "Worker" in the Executor Page of the web UI. For each executor (except the driver), it will provide a link to the web UI of the worker it belongs to.

This PR also retrieved worker URLs (besides worker UI URLs) from ExecutorRunner to ExecutorsPage. This would help if we want to show then on the Executor Page later. (See #14204 (comment))

Because the data of the URLs had not been retrieved to the executor UI (even not in the executor), passing them have the following effects:

The DeveloperApi ExecutorInfo is changed. A new argument, workerUrl: Map[String, String], is added in the constructor of that class. But it is set to Map.empty by default, so calling the old API still works. The executors for local and Mesos are stilling using call the old API.
The JSON converter of ExecutorInfo is update.
The REST API for executors has a new item call "worker", it looks like this:

 "worker" : {
"url" : "spark://Worker@192.168.1.104:32790",
"ui_url" : "http://192.168.1.104:46445"
}

How was this patch tested?

Run Spark as a "local cluster" by ./bin/spark-shell --master=local-cluster[2,1,1024]. Then view the Executor Page on http://localhost:4040/executors/.

You might need to clear the cache at the browser since some resources are changed. (especially /static/executorspage-template.html.)

See the column "Worker" on the right of the Executors Table. For each executor, click the hypertext link, jump to the web UI of the worker it belongs to.

Screenshot:

nblintao · 2016-07-14T16:06:22Z

It looks like this if we use the worker URL as the placeholder (instead of "Worker") in the column Worker. Please note the text (the URL of the worker) is different from its href (the URL of the Worker UI).

SparkQA · 2016-07-14T16:20:41Z

Test build #62327 has finished for PR 14204 at commit 21a5abc.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

nblintao · 2016-07-14T16:28:15Z

retest this please

SparkQA · 2016-07-14T16:52:12Z

Test build #62329 has finished for PR 14204 at commit 21a5abc.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

ajbozarth · 2016-07-19T00:00:58Z

I'll try to take a look at this tomorrow, but you're failing MiMa tests because you added a param to ExecutorInfo which is part of the developer api. You'll have to add an exclude to MimaExcludes for ExecutorInfo to pass MiMa.

nblintao · 2016-07-19T00:37:59Z

@ajbozarth Thanks for pointing out how to fix this. I'll try to fix it tomorrow.

Merge the updates

SparkQA · 2016-07-20T02:47:56Z

Test build #62572 has finished for PR 14204 at commit f2ab3a3.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-07-20T04:45:45Z

Test build #62575 has finished for PR 14204 at commit 4d1d47f.

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

ajbozarth · 2016-07-20T18:02:17Z

From some quick testing and look through of your code I think this will be good, but you'll need to redo work it a bit to work with the new executors page that was just merged. The page is displayed using JQuery DataTables now. I'll take a more detailed look after you update this.

# Conflicts: # core/src/main/scala/org/apache/spark/ui/exec/ExecutorsPage.scal1a

nblintao · 2016-07-21T14:48:49Z

I have fixed the conflicts with #13670.
Some unit tests for JSON protocol needs to be updated, but I cannot get assess to the page of Jenkins server somehow. I'll fix it as soon as I could visit Jenkins.

SparkQA · 2016-07-21T16:08:36Z

Test build #62680 has finished for PR 14204 at commit 801f8fe.

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

ajbozarth · 2016-07-21T21:16:51Z

Updates to the code look good but you need to update the two failing tests still. I also created SPARK-16673 for the issues discussed in #13670 and am mentioning it since it affects this pr's original intended functionality. I am ok with saying this LGTM once you address the tests and letting SPARK-16673 cover adding the display condition.

ajbozarth · 2016-07-21T23:51:21Z

@nblintao I'm not sure if you're aware but you can actually run the same tests Jenkins runs locally before pushing, there's a build/test script dev/run-tests, thought I'd mention it since you're test fixes didn't fix everything

nblintao · 2016-07-22T00:03:13Z

Thanks for your reminding, @ajbozarth. I committed without testing locally this time because I thought it was a small fix and could pass the test. Unluckily, it still needs to be updated.
I'll test locally before commit next time. Sorry if it spams.

ajbozarth · 2016-07-22T00:16:28Z

You're good, not everyone knows about dev/run-tests so I thought I'd mention it

SparkQA · 2016-07-22T00:54:10Z

Test build #62697 has finished for PR 14204 at commit 1618ffe.

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

ajbozarth · 2016-07-22T01:55:27Z

 val unpersistRdd = SparkListenerUnpersistRDD(12345)
 val logUrlMap = Map("stderr" -> "mystderr", "stdout" -> "mystdout").toMap
+ val workerUrlMap = Map("url" -> "spark://Worker@192.168.1.104:32790",
+ "ui_url" -> "http://192.168.1.104:46445").toMap


I feel like these ip's should be something more generic, maybe check if there any other places in the test code that have strings with ip and ports that you could copy? Not an actual problem though since whats in the string only has to match the string below.

Good idea. I've updated referring to the test cases in ClientSuite.scala. Thanks!

SparkQA · 2016-07-22T03:42:13Z

Test build #62700 has finished for PR 14204 at commit 519c329.

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

SparkQA · 2016-07-22T07:57:58Z

Test build #62719 has finished for PR 14204 at commit 48ae86f.

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

yhuai · 2016-07-24T23:21:02Z

@nblintao I tried ./bin/spark-shell --master=local-cluster[2,1,1024]. Seems those worker links do not show up? Maybe something has been changed and links do not show up anymore?

SparkQA · 2016-08-29T21:12:45Z

Test build #64583 has finished for PR 14204 at commit bed0310.

This patch fails from timeout after a configured wait of 250m.
This patch merges cleanly.
This patch adds no public classes.

nblintao · 2016-08-29T22:43:00Z

Interesting, test again please.

nblintao · 2016-08-30T01:53:57Z

retest this, please

SparkQA · 2016-08-30T04:16:18Z

Test build #64615 has finished for PR 14204 at commit bed0310.

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

ajbozarth · 2016-10-13T21:56:47Z

@nblintao Are you willing to rebase this and see if we can try to get it merged?

SparkQA · 2016-12-29T02:18:30Z

Test build #70696 has finished for PR 14204 at commit 2d6d2f2.

This patch fails RAT tests.
This patch merges cleanly.
This patch adds no public classes.

nblintao · 2016-12-29T02:54:51Z

@ajbozarth I finally have a chance to rebase it in the winter break. Could you please have a look? Thanks!

SparkQA · 2016-12-29T04:51:34Z

Test build #70697 has finished for PR 14204 at commit 11fe868.

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

ajbozarth

Just one comment for now, I'll try to check this out and make sure it still looks good sometime this week.

ajbozarth · 2017-01-03T18:32:11Z

+ },
+ {
+ "targets": [ 17 ],
+ "visible": workersExist(response)


We ran into an issue recently with columnDefs, it only runs on initial page load and doesn't catch changes on refresh. So for columns with non-static visibility you should add a line like this one below dt.column(15).visible(logsExist(response));

Fixed. Thank you!

Thanks, but this code is redundant now so it should be removed

Fixed. Thanks!

SparkQA · 2017-01-03T23:15:09Z

Test build #70825 has finished for PR 14204 at commit 3680eb5.

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

ajbozarth · 2017-01-18T00:02:46Z

@srowen @vanzin could you take a look at this, I believe I've given all the feedback I can think of

SparkQA · 2017-01-18T05:02:57Z

Test build #71557 has finished for PR 14204 at commit d23643c.

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

vanzin · 2017-01-18T19:21:38Z

So, if I understand this correctly, in non-standalone mode you'll have a completely empty column in the UI taking up space?

Why is it important to collect the worker URL (not the UI address)? There's nothing users can do with that.

I'm also not a fan of exposing things that are particular to one specific cluster manager in the REST API.

I'd be a little more ok with this patch if it hid that information from the UI when it's not available.

But in fact I don't really see much value in exposing that link. To me, it just clutters up the UI, so you might as well just have a link to the Master UI somewhere and tell users to find the worker there if they need it.

ajbozarth · 2017-01-18T21:17:12Z

@vanzin it sounds like you'd rather just have #14461 and not add this. As for the visibility issue, the column is only visible if it has links available. And I can agree about the REST API issue, but given we're moving towards using the API to surface all data to the web ui that seems unavoidable.

vanzin · 2017-01-18T21:20:40Z

it sounds like you'd rather just have #14461 and not add this

That's one way of looking at it (although I haven't looked at that change). I took a quick look at the worker UI and didn't see anything interesting there. It has links to the executor's log files, but those already exist in the app UI.

ajbozarth · 2017-01-18T21:31:56Z

@nblintao @yhuai what was your original intent with this PR? Because based on @vanzin comments I'm also leaning towards dropping this.

yhuai · 2017-01-19T00:25:26Z

ok I agree. Originally, I thought it will be helpful to figure out the worker that an executor belongs to.

But, if it does not provide very useful information. I am fine to drop it.

ajbozarth · 2017-01-19T00:34:06Z

@nblintao If you could close this and @vanzin could you take a look at #14461 ?

Closes#15736Closes#16309Closes#16485Closes#16502Closes#16196Closes#16498Closes#12380Closes#16764Closes#14394Closes#14204Closes#14027Closes#13690Closes#16279 Author: Sean Owen <sowen@cloudera.com> Closes#16778 from srowen/CloseStalePRs.

Closesapache#15736Closesapache#16309Closesapache#16485Closesapache#16502Closesapache#16196Closesapache#16498Closesapache#12380Closesapache#16764Closesapache#14394Closesapache#14204Closesapache#14027Closesapache#13690Closesapache#16279 Author: Sean Owen <sowen@cloudera.com> Closesapache#16778 from srowen/CloseStalePRs.

add links to worker UIs in the executor page
21a5abc

Merge branch 'master' into linkExecutorToWorker
f2ab3a3
Merge the updates

mima excludes
4d1d47f

Merge branch 'master' into linkExecutorToWorker
801f8fe
# Conflicts: # core/src/main/scala/org/apache/spark/ui/exec/ExecutorsPage.scal1a

nblintao mentioned this pull request Jul 21, 2016
[SPARK-15951] Change Executors Page to use datatables to support sorting columns and searching #13670
Closed

Fix for the unit test
1618ffe

Fix JsonProtocolSuite
519c329

ajbozarth reviewed Jul 22, 2016
View reviewed changes

make the test case more generic
48ae86f

nblintao changed the title ~~[SPARK-16520] [WEBUI] Link executors to corresponding worker pages~~[SPARK-16520][WEBUI] Link executors to corresponding worker pagesJul 22, 2016

nblintao changed the title ~~[SPARK-16520][WEBUI] Link executors to corresponding worker pages~~[SPARK-16520] [WEBUI] Link executors to corresponding worker pagesJul 22, 2016

nblintao added 2 commits December 28, 2016 20:09

Merge remote-tracking branch 'apache/master'
8743898

resolve conflicts
2d6d2f2

nblintao added 2 commits December 28, 2016 21:22

miss a comma
2732db8

remove a local file
11fe868

ajbozarth reviewed Jan 3, 2017
View reviewed changes

nblintao added 3 commits January 3, 2017 14:46

Merge remote-tracking branch 'upstream/master'
3dfd0f0

Merge branch 'master' into linkExecutorToWorker
ae4794a

catch changes on refresh
3680eb5

remove unnecessary code
d23643c

srowen mentioned this pull request Feb 2, 2017
[BUILD] Close stale PRs #16778
Closed

nblintao closed this Feb 2, 2017

Uh oh!

Conversation

nblintao commented Jul 14, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

nblintao commented Jul 14, 2016

Uh oh!

SparkQA commented Jul 14, 2016

Uh oh!

nblintao commented Jul 14, 2016

Uh oh!

SparkQA commented Jul 14, 2016

Uh oh!

ajbozarth commented Jul 19, 2016

Uh oh!

nblintao commented Jul 19, 2016

Uh oh!

SparkQA commented Jul 20, 2016

Uh oh!

SparkQA commented Jul 20, 2016

Uh oh!

ajbozarth commented Jul 20, 2016

Uh oh!

nblintao commented Jul 21, 2016

Uh oh!

SparkQA commented Jul 21, 2016

Uh oh!

ajbozarth commented Jul 21, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajbozarth commented Jul 21, 2016

Uh oh!

nblintao commented Jul 22, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajbozarth commented Jul 22, 2016

Uh oh!

SparkQA commented Jul 22, 2016

Uh oh!

ajbozarthJul 22, 2016

Choose a reason for hiding this comment

Uh oh!

nblintaoJul 22, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 22, 2016

Uh oh!

SparkQA commented Jul 22, 2016

Uh oh!

yhuai commented Jul 24, 2016

Uh oh!

SparkQA commented Aug 29, 2016

Uh oh!

nblintao commented Aug 29, 2016

Uh oh!

nblintao commented Aug 30, 2016

Uh oh!

SparkQA commented Aug 30, 2016

Uh oh!

ajbozarth commented Oct 13, 2016

Uh oh!

SparkQA commented Dec 29, 2016

Uh oh!

nblintao commented Dec 29, 2016

Uh oh!

SparkQA commented Dec 29, 2016

Uh oh!

ajbozarth left a comment

Choose a reason for hiding this comment

Uh oh!

ajbozarthJan 3, 2017

Choose a reason for hiding this comment

Uh oh!

nblintaoJan 3, 2017

Choose a reason for hiding this comment

Uh oh!

ajbozarthJan 18, 2017

Choose a reason for hiding this comment

nblintao commented Jul 14, 2016•
edited
Loading

ajbozarth commented Jul 21, 2016•
edited
Loading

nblintao commented Jul 22, 2016•
edited
Loading