[SPARK-29599][WEBUI] Support pagination for session table in JDBC/ODBC Tab #26253

AngersZhuuuu · 2019-10-25T08:21:12Z

What changes were proposed in this pull request?

In this PR, extend the support of pagination to session table in JDBC/PDBC .

Why are the changes needed?

Some times we may connect a lot client and there a many session info shown in session tab.
make it can be paged for better view.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Manuel verify.
After pr:

AngersZhuuuu · 2019-10-25T08:21:34Z

cc @srowen @wangyum

wangyum · 2019-10-25T08:33:33Z

ok to test

SparkQA · 2019-10-25T08:58:15Z

Test build #112658 has finished for PR 26253 at commit dfa225b.

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

srowen · 2019-10-25T13:08:51Z

...thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/ui/ThriftServerPage.scala

+
+    override def sliceData(from: Int, to: Int): Seq[SessionInfo] = {
+      val r = data.slice(from, to)
+      r.map(x => x)


Nit: what does this do?

Nit: what does this do?

For paging, get selected pages data.

No, I mean, .map(x => x) is a no-op right? what is this line for?

No, I mean, .map(x => x) is a no-op right? what is this line for?

Same implement with #26215

Since same function... so don't check this method.
Remove both of these.

Yes, we can remove the line.

shahidki31 · 2019-10-25T13:45:41Z

...thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/ui/ThriftServerPage.scala

+  }
+
+  private def errorMessageCell(errorMessage: String): Seq[Node] = {
+    val isMultiline = errorMessage.indexOf('\n') >= 0


Is this method used anywhere?

Is this method used anywhere?

All remove

shahidki31 · 2019-10-25T13:46:05Z

...thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/ui/ThriftServerPage.scala

+    </td>
+  }
+
+  private def jobURL(request: HttpServletRequest, jobId: String): String =


This method also seems not used?

shahidki31 · 2019-10-25T13:48:34Z

...thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/ui/ThriftServerPage.scala

+
+    override def sliceData(from: Int, to: Int): Seq[SessionInfo] = {
+      val r = data.slice(from, to)
+      r.map(x => x)


Yes, we can remove the line.

shahidki31 · 2019-10-25T14:03:13Z

...thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/ui/ThriftServerPage.scala

+    val sessionTableHeaders =
+      Seq("User", "IP", "Session ID", "Start Time", "Finish Time", "Duration", "Total Execute")
+
+    val tooltips = Seq[Option[String]](None, None, None, None, None, None, None)


Do we need tooltips for the table?

Do we need tooltips for the table?

Ok for me to remove these. If we all think don't need and won't add some thing, just remove it.
By the way, thanks a lot for you nice job. This problem trouble me a lot

I think there is already a PR working on it, related to tool tips here, #26138

I think there is already a PR working on it, related to tool tips here, #26138

Very embarrassed..., for me , this table's information is so clear. Don't add tooltip is ok.

shahidki31 · 2019-10-25T14:25:35Z

...thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/ui/ThriftServerPage.scala

+      <td> {formatDate(session.startTimestamp)} </td>
+      <td> {if (session.finishTimestamp > 0) formatDate(session.finishTimestamp)} </td>
+      <td sorttable_customkey={session.totalTime.toString}>
+        {formatDurationOption(Some(session.totalTime))} </td>


I think, we can directly call UIUtils.formatDurationVerbose , because totalTime is always defined. Also can remove the method formatDurationOption

I think, we can directly call UIUtils.formatDurationVerbose , because totalTime is always defined. Also can remove the method formatDurationOption

Good suggestion. Removed other old unused code, feel comfortable now.

SparkQA · 2019-10-25T14:29:20Z

Test build #112669 has finished for PR 26253 at commit f209cb4.

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

SparkQA · 2019-10-25T15:16:19Z

Test build #112675 has finished for PR 26253 at commit c7c9479.

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

shahidki31 · 2019-10-25T17:35:32Z

...thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/ui/ThriftServerPage.scala

+      }.getOrElse("Start Time")
+      val sessionTableSortDesc = Option(parameterSessionTableSortDesc).map(_.toBoolean).getOrElse(
+        // Old session should be shown above new session by default.
+        !(sessionTableSortColumn == "Start Time")


@AngersZhuuuu By default new session should come above right? (Screenshot from master branch)

@AngersZhuuuu By default new session should come above right? (Screenshot from master branch)

Updated, I could have been misled by the modified thriftserver I was using

shahidki31

LGTM, pending one comment.

SparkQA · 2019-10-25T20:04:40Z

Test build #112687 has finished for PR 26253 at commit ce588e5.

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

srowen · 2019-10-26T20:47:40Z

Just need to resolve the merge conflict too

SparkQA · 2019-10-27T03:50:28Z

Test build #112723 has finished for PR 26253 at commit 59697bc.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
abstract class BitAggregate extends DeclarativeAggregate with ExpectsInputTypes
case class BitAndAgg(child: Expression) extends BitAggregate
case class BitOrAgg(child: Expression) extends BitAggregate
case class BitXorAgg(child: Expression) extends BitAggregate
case class Version() extends LeafExpression with CodegenFallback
case class ShowCreateTableStatement(tableName: Seq[String]) extends ParsedStatement

AngersZhuuuu · 2019-10-27T03:59:28Z

Just need to resolve the merge conflict too

Done

shahidki31 · 2019-10-27T14:19:53Z

...thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/ui/ThriftServerPage.scala

+      <td> <a href={sessionLink}> {session.sessionId} </a> </td>
+      <td> {formatDate(session.startTimestamp)} </td>
+      <td> {if (session.finishTimestamp > 0) formatDate(session.finishTimestamp)} </td>
+      <td sorttable_customkey={session.totalTime.toString}>


nit: Is it necessary to add sorttable_customkey as we are already sorting using raw data in ordering method?

nit: Is it necessary to add sorttable_customkey as we are already sorting using raw data in ordering method?

This seems to be the previous pr, change to use raw data for sort column, old version didn't have this and trouble me a lot. Don't need it.

Also, please confirm if the sorting is working properly after changing this. Thanks

Also, please confirm if the sorting is working properly after changing this. Thanks

Checked ok. Thanks

shahidki31 · 2019-10-27T14:22:16Z

...thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/ui/ThriftServerPage.scala

      }
-      Some(UIUtils.listingTable(headerRow, generateDataRow, dataRows, true, None, Seq(null), false))


nit: Here for the sessions table fixedWidth is true. right? Are we making the paged table also fixed width?

fixedWidth

Changed. Not so familiar with UI HTML part, thanks a lot for your suggestion

SparkQA · 2019-10-27T15:06:52Z

Test build #112731 has finished for PR 26253 at commit 691eed1.

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

SparkQA · 2019-10-27T15:44:38Z

Test build #112732 has finished for PR 26253 at commit 5315c58.

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

srowen · 2019-10-28T13:45:40Z

Merged to master

Update ThriftServerPage.scala

dfa225b

srowen reviewed Oct 25, 2019

View reviewed changes

shahidki31 reviewed Oct 25, 2019

View reviewed changes

Update ThriftServerPage.scala

f209cb4

shahidki31 reviewed Oct 25, 2019

View reviewed changes

AngersZhuuuu added 2 commits October 25, 2019 22:45

remove unused code

43a1621

Update ThriftServerPage.scala

c7c9479

shahidki31 reviewed Oct 25, 2019

View reviewed changes

shahidki31 approved these changes Oct 25, 2019

View reviewed changes

Update ThriftServerPage.scala

ce588e5

dongjoon-hyun added the WEB UI label Oct 25, 2019

Merge branch 'master' into SPARK-29599

59697bc

shahidki31 reviewed Oct 27, 2019

View reviewed changes

follow comment

691eed1

fix bug

5315c58

shahidki31 approved these changes Oct 27, 2019

View reviewed changes

srowen approved these changes Oct 28, 2019

View reviewed changes

srowen closed this in d6e33dc Oct 28, 2019

		}
		Some(UIUtils.listingTable(headerRow, generateDataRow, dataRows, true, None, Seq(null), false))

[SPARK-29599][WEBUI] Support pagination for session table in JDBC/ODBC Tab #26253

[SPARK-29599][WEBUI] Support pagination for session table in JDBC/ODBC Tab #26253

Conversation

AngersZhuuuu commented Oct 25, 2019

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

AngersZhuuuu commented Oct 25, 2019

wangyum commented Oct 25, 2019

SparkQA commented Oct 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahidki31 Oct 25, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Oct 25, 2019

SparkQA commented Oct 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahidki31 left a comment

Choose a reason for hiding this comment

SparkQA commented Oct 25, 2019

srowen commented Oct 26, 2019

SparkQA commented Oct 27, 2019

AngersZhuuuu commented Oct 27, 2019

shahidki31 Oct 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AngersZhuuuu Oct 27, 2019 • edited

Choose a reason for hiding this comment

SparkQA commented Oct 27, 2019

SparkQA commented Oct 27, 2019

srowen commented Oct 28, 2019

shahidki31 Oct 25, 2019 •

edited

shahidki31 Oct 27, 2019 •

edited

AngersZhuuuu Oct 27, 2019 •

edited