Skip to content
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.

Sign up for GitHub

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

[SPARK-21250][WEB-UI]Add a url in the table of 'Running Executors' in worker page to visit job page. #18464

Closed
wants to merge 3 commits into from
Closed

[SPARK-21250][WEB-UI]Add a url in the table of 'Running Executors' in worker page to visit job page. #18464

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6823d73
[SPARK-21250]Add a url in the table of 'Running Executors' in worker …
Jun 29, 2017
aa269a6
[SPARK-21250]fix scala style
Jun 30, 2017
1411ed9
Make a non - empty judgment.For a spark shell, we can disable WebUI l…
Jul 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions core/src/main/scala/org/apache/spark/deploy/worker/ui/WorkerPage.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import scala.xml.Node
import org.json4s.JValue

import org.apache.spark.deploy.DeployMessages.{RequestWorkerState, WorkerStateResponse}
import org.apache.spark.deploy.JsonProtocol
import org.apache.spark.deploy.{ExecutorState, JsonProtocol}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @guoxiaolongzte .
This is a scalastyle violation.
Could you do dev/scalastyle?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have fixed it, thanks.

import org.apache.spark.deploy.master.DriverState
import org.apache.spark.deploy.worker.{DriverRunner, ExecutorRunner}
import org.apache.spark.ui.{UIUtils, WebUIPage}
Expand Down Expand Up @@ -112,7 +112,15 @@ private[ui] class WorkerPage(parent: WorkerWebUI) extends WebUIPage("") {
<td>
<ul class="unstyled">
<li><strong>ID:</strong> {executor.appId}</li>
<li><strong>Name:</strong> {executor.appDesc.name}</li>
<li><strong>Name:</strong>
{
if ({executor.state == ExecutorState.RUNNING}) {
<a href={executor.appDesc.appUiUrl}> {executor.appDesc.name}</a>
Copy link
Member

@dongjoon-hyun dongjoon-hyun Jun 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if exeutor.appDesc.appUIUrl is empty?
Could you add prevention logic at above if statement not to add invalid hyperlink like the following?

<a href="">..</a>

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if exeutor.appDesc.appUIUrl is empty, click the URL, the jump page is the current page.
Of course the task is running, exeutor.appDesc.appUIUrl can not be empty.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • For me, click the URL, the jump page is the current page. sounds not a good UX.
  • When you use spark-shell, what happens on exeutor.appDesc.appUIUrl?

Copy link
Author

@guoxiaolongzte guoxiaolongzte Jun 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The jump page is the current page. sounds not a good UX. I can add a non-empty judgment.

  2. When I use spark-shell, exeutor.appDesc.appUIUrl equal '10.43.183.117:4040'

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When will executor.appDesc.appUiUrl be empty?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was originally set by:

    val webUrl = sc.ui.map(_.webUrl).getOrElse("")

The webUrl is only valid after bind(). But I think normally this should not be empty.

Copy link
Author

@guoxiaolongzte guoxiaolongzte Jun 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I also think so. @dongjoon-hyun Is it necessary to make a non - empty judgment?

{
        if ({executor.state == ExecutorState.RUNNING && executor.appDesc.appUiUrl.nonEmpty}) {
          <a href={executor.appDesc.appUiUrl}> {executor.appDesc.name}</a>
        } else {
          {executor.appDesc.name}
        }
      }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @cloud-fan , @jiangxb1987 , @guoxiaolongzte .
For a spark shell, we can disable WebUI like the following. In that case, that will be empty always.

bin/spark-shell --conf spark.ui.enabled=false

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dongjoon-hyun I missed that in my review, we definitely need that non-empty check

} else {
{executor.appDesc.name}
}
}
</li>
<li><strong>User:</strong> {executor.appDesc.user}</li>
</ul>
</td>
Expand Down