Skip to content

[SPARK-29019][WebUI] Improve tooltip JDBC/ODBC Server tab #25723

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

Closed
planga82 wants to merge 16 commits into from
Closed

[SPARK-29019][WebUI] Improve tooltip JDBC/ODBC Server tab #25723

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
bc0adbc
Merge remote-tracking branch 'upstream/master'
planga82 Aug 11, 2019
6f213ce
Merge remote-tracking branch 'upstream/master'
planga82 Aug 12, 2019
cd193a9
Merge remote-tracking branch 'upstream/master'
planga82 Aug 24, 2019
247d667
Merge remote-tracking branch 'upstream/master'
planga82 Sep 2, 2019
5956468
Merge remote-tracking branch 'upstream/master'
planga82 Sep 7, 2019
c4a209f
[SPARK-29019][WebUI] Improve tooltip JDBC/ODBC Server tab
planga82 Sep 8, 2019
681b534
[SPARK-29019][WebUI] Fix scala style
planga82 Sep 9, 2019
a95c90b
[SPARK-29019][WebUI] Fix typo and text
planga82 Sep 10, 2019
2d278d0
[SPARK-29019][WebUI] Update ThiftServerSessionPage
planga82 Sep 10, 2019
793ef92
[SPARK-29019][WebUI] Fix white space scala style
planga82 Sep 10, 2019
7f480f8
[SPARK-29019][WebUI] Fix tooltip description
planga82 Sep 10, 2019
344fd6f
[SPARK-29019][WebUI] Improve comments
planga82 Sep 10, 2019
6474935
[SPARK-29019][WebUI] Assert equal lenght parameters
planga82 Sep 11, 2019
1bb6196
[SPARK-29019] Move tooltips to thrift server module
planga82 Sep 26, 2019
6ab97b8
[SPARK-29019] Fix scala style
planga82 Sep 26, 2019
281c8ce
[SPARK-29019] delete blank line
planga82 Sep 27, 2019
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: 12 additions & 0 deletions core/src/main/scala/org/apache/spark/ui/ToolTips.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,4 +99,16 @@ private[spark] object ToolTips {
dynamic allocation is enabled. The number of granted executors may exceed the limit
ephemerally when executors are being killed.
"""

val THRIFT_SERVER_FINISH_TIME =
Comment thread
dongjoon-hyun marked this conversation as resolved.
Outdated

@wangyum wangyum Sep 25, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we move these changes and tests to hive-thriftserver module? @srowen @dongjoon-hyun What do you think?

@srowen srowen Sep 25, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, if these are only relevant to the thrift server UI, they could go with the other thrift server UI code.

@dongjoon-hyun dongjoon-hyun Sep 25, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1, too.

@planga82 planga82 Sep 26, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, it make sense to have the tooltips in thriftserver module, i'm going to move it.
To generate the tables for this pages we use UIUtils from spark core module so I needed to improve this utils to add this new functionality.
Also, this improvements could be use in other pages to add tooltips to the tables.

"Finish time of the execution, before fetching the results"

@juliuszsompolski juliuszsompolski Sep 10, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think "Execution finish time" would sound better than "Finish time of the execution"

@planga82 planga82 Sep 10, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm Sorry, spanish common error when we write in english :-)


val THRIFT_SERVER_CLOSE_TIME =
"Close time of the operation after fetching the results"

@juliuszsompolski juliuszsompolski Sep 10, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Likewise, "Operation close time"


val THRIFT_SERVER_EXECUTION =
"Difference between start time and finish time"

val THRIFT_SERVER_DURATION =
"Difference between start time and close time"
}
22 changes: 20 additions & 2 deletions core/src/main/scala/org/apache/spark/ui/UIUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,9 @@ private[spark] object UIUtils extends Logging {
id: Option[String] = None,
headerClasses: Seq[String] = Seq.empty,
stripeRowsWithCss: Boolean = true,
sortable: Boolean = true): Seq[Node] = {
sortable: Boolean = true,
// The tooltip information could be None, which indicates header does not have a tooltip.
tooltipHeaders: Seq[Option[String]] = Seq.empty): Seq[Node] = {
Comment thread
srowen marked this conversation as resolved.
Comment thread
srowen marked this conversation as resolved.

val listingTableClass = {
val _tableClass = if (stripeRowsWithCss) TABLE_CLASS_STRIPED else TABLE_CLASS_NOT_STRIPED
Expand All @@ -332,6 +334,14 @@ private[spark] object UIUtils extends Logging {
}
}

def getTooltip(index: Int): Option[String] = {
if (index < tooltipHeaders.size) {
tooltipHeaders(index)
} else {
None
}
}

val newlinesInHeader = headers.exists(_.contains("\n"))
def getHeaderContent(header: String): Seq[Node] = {
if (newlinesInHeader) {
Expand All @@ -345,7 +355,15 @@ private[spark] object UIUtils extends Logging {

val headerRow: Seq[Node] = {
headers.view.zipWithIndex.map { x =>
<th width={colWidthAttr} class={getClass(x._2)}>{getHeaderContent(x._1)}</th>
getTooltip(x._2) match {
case Some(tooltip) =>
<th width={colWidthAttr} class={getClass(x._2)}>
<span data-toggle="tooltip" title={tooltip}>
{getHeaderContent(x._1)}
</span>
</th>
case None => <th width={colWidthAttr} class={getClass(x._2)}>{getHeaderContent(x._1)}</th>
}
}
}
<table class={listingTableClass} id={id.map(Text.apply)}>
Expand Down
50 changes: 50 additions & 0 deletions core/src/test/scala/org/apache/spark/ui/UIUtilsSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
package org.apache.spark.ui

import scala.xml.{Node, Text}
import scala.xml.Utility.trim

import org.apache.spark.SparkFunSuite

Expand Down Expand Up @@ -129,6 +130,55 @@ class UIUtilsSuite extends SparkFunSuite {
assert(decoded1 === decodeURLParameter(decoded1))
}

test("listingTable with tooltips") {

def generateDataRowValue: String => Seq[Node] = row => <a>{row}</a>
val header = Seq("Header1", "Header2")
val data = Seq("Data1", "Data2")
val tooltip = Seq(None, Some("tooltip"))

val generated = listingTable(header, generateDataRowValue, data, tooltipHeaders = tooltip)

val expected: Node =
<table class="table table-bordered table-condensed table-striped sortable">
<thead>
<th width="" class="">{header(0)}</th>
<th width="" class="">
<span data-toggle="tooltip" title="tooltip">
{header(1)}
</span>
</th>
</thead>
<tbody>
{data.map(generateDataRowValue)}
</tbody>
</table>

assert(trim(generated(0)) == trim(expected))
}

test("listingTable without tooltips") {

def generateDataRowValue: String => Seq[Node] = row => <a>{row}</a>
val header = Seq("Header1", "Header2")
val data = Seq("Data1", "Data2")

val generated = listingTable(header, generateDataRowValue, data)

val expected =
<table class="table table-bordered table-condensed table-striped sortable">
<thead>
<th width="" class="">{header(0)}</th>
<th width="" class="">{header(1)}</th>
</thead>
<tbody>
{data.map(generateDataRowValue)}
</tbody>
</table>

assert(trim(generated(0)) == trim(expected))
}

private def verify(
desc: String,
expected: Node,
Expand Down
5 changes: 4 additions & 1 deletion ...iftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/ui/ThriftServerPage.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@ private[ui] class ThriftServerPage(parent: ThriftServerTab) extends WebUIPage(""
val table = if (numStatement > 0) {
val headerRow = Seq("User", "JobID", "GroupID", "Start Time", "Finish Time", "Close Time",
"Execution Time", "Duration", "Statement", "State", "Detail")
val tooltips = Seq(None, None, None, None, Some(ToolTips.THRIFT_SERVER_FINISH_TIME),

@juliuszsompolski juliuszsompolski Sep 10, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we add the same to ThriftServerSessionPage?

@planga82 planga82 Sep 10, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought it was the same code to generate this two pages. I forgot test it. Thanks!!

Some(ToolTips.THRIFT_SERVER_CLOSE_TIME), Some(ToolTips.THRIFT_SERVER_EXECUTION),
Some(ToolTips.THRIFT_SERVER_DURATION), None, None, None)

@juliuszsompolski juliuszsompolski Sep 10, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Or, if not inside UIUtils.listingTable, maybe add assert(headerRow.length == tooltips.length) here and in ThriftServerSessionPage?

@planga82 planga82 Sep 11, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've been thinking about it, yes, it's a good idea, in that way we don't break the homogeneous way of treating the parameters at UIUtils.listingTable and we ensure it is going to be less problems if new headers are added

val dataRows = listener.getExecutionList.sortBy(_.startTimestamp).reverse

def generateDataRow(info: ExecutionInfo): Seq[Node] = {
Expand Down Expand Up @@ -100,7 +103,7 @@ private[ui] class ThriftServerPage(parent: ThriftServerTab) extends WebUIPage(""
}

Some(UIUtils.listingTable(headerRow, generateDataRow,
dataRows, false, None, Seq(null), false))
dataRows, false, None, Seq(null), false, tooltipHeaders = tooltips))
} else {
None
}
Expand Down
5 changes: 4 additions & 1 deletion ...er/src/main/scala/org/apache/spark/sql/hive/thriftserver/ui/ThriftServerSessionPage.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,9 @@ private[ui] class ThriftServerSessionPage(parent: ThriftServerTab)
val table = if (numStatement > 0) {
val headerRow = Seq("User", "JobID", "GroupID", "Start Time", "Finish Time", "Close Time",
"Execution Time", "Duration", "Statement", "State", "Detail")
val tooltips = Seq(None, None, None, None, Some(ToolTips.THRIFT_SERVER_FINISH_TIME),
Some(ToolTips.THRIFT_SERVER_CLOSE_TIME), Some(ToolTips.THRIFT_SERVER_EXECUTION),
Some(ToolTips.THRIFT_SERVER_DURATION), None, None, None)
val dataRows = executionList.sortBy(_.startTimestamp).reverse

def generateDataRow(info: ExecutionInfo): Seq[Node] = {
Expand Down Expand Up @@ -109,7 +112,7 @@ private[ui] class ThriftServerSessionPage(parent: ThriftServerTab)
}

Some(UIUtils.listingTable(headerRow, generateDataRow,
dataRows, false, None, Seq(null), false))
dataRows, false, None, Seq(null), false, tooltipHeaders = tooltips))
} else {
None
}
Expand Down