[SPARK-31688][WEBUI] Refactor Pagination framework #28512

iRakson · 2020-05-12T18:09:11Z

What changes were proposed in this pull request?

Currently while implementing pagination using the existing pagination framework, a lot of code is being copied as pointed out here.

I introduced some changes in PagedTable which is the main trait for implementing the pagination.

Added function for getting table parameters.
Added a function for table header row. This will help in maintaining consistency across the tables. All the header rows across tables will be consistent now.

Why are the changes needed?

A lot of code is copied every time pagination is implemented for any table.
Code readability is not great as lot of HTML is embedded.
Paginating other tables will be a lot easier now.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Manually. This is mainly refactoring work, no new functionality introduced. Existing test cases should pass.

iRakson · 2020-05-12T18:11:36Z

cc @srowen @uncleGen @sarutak @HyukjinKwon Kindly review.

sarutak · 2020-05-12T21:13:53Z

ok to test.

SparkQA · 2020-05-12T21:46:08Z

Test build #122565 has finished for PR 28512 at commit 7775dd1.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

uncleGen · 2020-05-13T02:32:46Z

retest this please.

uncleGen · 2020-05-13T02:39:04Z

core/src/main/scala/org/apache/spark/ui/PagedTable.scala

+  def getTableParameters(request: HttpServletRequest, tableTag: String,
+      defaultSortColumn: String): (String, Boolean, Int) = {


nit: all arguments in one line or each one in separate line.

SparkQA · 2020-05-13T03:03:53Z

Test build #122570 has finished for PR 28512 at commit 7775dd1.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-05-13T12:13:12Z

Test build #122591 has finished for PR 28512 at commit 89194d3.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-05-13T12:57:33Z

Test build #122594 has finished for PR 28512 at commit 0f886d6.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

iRakson · 2020-05-14T05:57:33Z

retest this please

iRakson · 2020-05-16T19:20:23Z

@srowen @sarutak @uncleGen If Jenkins is fine, can we test this ?

srowen · 2020-05-16T19:50:00Z

Jenkins test this please

SparkQA · 2020-05-17T00:19:59Z

Test build #122736 has finished for PR 28512 at commit 0f886d6.

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

iRakson · 2020-05-18T16:00:43Z

@srowen @sarutak @uncleGen Can you take a look at this PR.
#28485 and #28439 are pending due to this one.

srowen · 2020-05-18T16:47:24Z

I don't know enough to have a strong opinion, but it looks plausible and I think you know this code well. If it doesn't introduce any behavior change, just a cleanup, I think it's OK.

sarutak · 2020-05-20T08:49:17Z

Now I'm checking and will leave comment.

sarutak

Thanks @iRakson for the refactoring!
It almost LGTM.
I've left some minor comments.

sarutak · 2020-05-20T08:58:19Z

core/src/main/scala/org/apache/spark/ui/jobs/AllJobsPage.scala

  }

  override def headers: Seq[Node] = {
    // Information for each header: title, cssClass, and sortable


Could you update the comment?

sarutak · 2020-05-20T09:01:57Z

core/src/main/scala/org/apache/spark/ui/PagedTable.scala

+   */
+  def isSortColumnValid(
+      headerInfo: Seq[(String, Boolean, Option[String])],
+      sortColumn: String): Any = {


The return type is better to be Unit rather than Any isn't it?

sarutak · 2020-05-20T09:55:34Z

core/src/main/scala/org/apache/spark/ui/jobs/StageTable.scala

    isFairScheduler: Boolean,
    killEnabled: Boolean,
    isFailedStage: Boolean) {
-  val parameterOtherTable = request.getParameterMap().asScala


JavaConverters._ is imported but it may be no longer used after this change.

sarutak · 2020-05-20T09:56:38Z

core/src/main/scala/org/apache/spark/ui/jobs/AllJobsPage.scala

      jobTag: String,
      jobs: Seq[v1.JobData],
      killEnabled: Boolean): Seq[Node] = {
-    val parameterOtherTable = request.getParameterMap().asScala


Same comment as here.

sarutak · 2020-05-20T09:57:52Z

core/src/main/scala/org/apache/spark/ui/jobs/AllJobsPage.scala

-    val headerRow: Seq[Node] = {
-      jobHeadersAndCssClasses.map { case (header, cssClass, sortable, tooltip) =>
-        if (header == sortColumn) {
-          val headerLink = Unparsed(


Unparsed is imported but it's no longer used after this change.

sarutak · 2020-05-20T09:59:44Z

sql/core/src/main/scala/org/apache/spark/sql/execution/ui/AllExecutionsPage.scala

  }

  override def headers: Seq[Node] = {
    // Information for each header: title, sortable


Same comment as here.

sarutak · 2020-05-20T09:59:58Z

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

-    val headerRow: Seq[Node] = {
-      sqlTableHeaders.zip(tooltips).map { case (header, tooltip) =>
-        if (header == sortColumn) {
-          val headerLink = Unparsed(


Same comment as here.

sarutak · 2020-05-20T10:00:55Z

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


      val sqlTableTag = "sqlsessionstat"

-      val parameterOtherTable = request.getParameterMap().asScala


Same comment as here.

sarutak · 2020-05-20T10:42:36Z

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

-              s"&$sessionStatsTableTag.pageSize=$pageSize" +
-              s"#$sessionStatsTableTag")
-          val arrow = if (desc) "&#x25BE;" else "&#x25B4;" // UP or DOWN
-            <th width={colWidthAttr}>


PagedTable#headerRow you added doesn't support the width of th.
Did you confirm it doesn't matter for ThriftServerPage?

Yeah, its working fine.

sarutak · 2020-05-20T10:44:20Z

core/src/main/scala/org/apache/spark/ui/PagedTable.scala

+      }
+    }
+    <thead>
+      {row}


{row} should be enclosed by <tr>.

iRakson · 2020-05-20T13:26:05Z

@sarutak Thank You for reviewing. Code is updated as per suggestions.

SparkQA · 2020-05-20T16:20:08Z

Test build #122899 has finished for PR 28512 at commit 50065ff.

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

srowen · 2020-05-21T18:00:07Z

Merged to master

iRakson · 2020-05-21T18:06:29Z

Thank You. 😊 @srowen @sarutak

[SPARK-31688] refactor pagination framework

7775dd1

probot-autolabeler bot added CORE SQL WEB UI labels May 12, 2020

uncleGen mentioned this pull request May 13, 2020

[SPARK-31642] Add Pagination Support for Structured Streaming Page #28485

Closed

uncleGen reviewed May 13, 2020

View reviewed changes

refactor thriftserverPage

89194d3

fix

0f886d6

sarutak reviewed May 20, 2020

View reviewed changes

fix

50065ff

iRakson requested a review from sarutak May 20, 2020 13:26

sarutak approved these changes May 21, 2020

View reviewed changes

srowen closed this in f1495c5 May 21, 2020

		def getTableParameters(request: HttpServletRequest, tableTag: String,
		defaultSortColumn: String): (String, Boolean, Int) = {


		val sqlTableTag = "sqlsessionstat"

		val parameterOtherTable = request.getParameterMap().asScala

[SPARK-31688][WEBUI] Refactor Pagination framework #28512

[SPARK-31688][WEBUI] Refactor Pagination framework #28512

Uh oh!

Conversation

iRakson commented May 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

iRakson commented May 12, 2020

Uh oh!

sarutak commented May 12, 2020

Uh oh!

SparkQA commented May 12, 2020

Uh oh!

uncleGen commented May 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 13, 2020

Uh oh!

SparkQA commented May 13, 2020

Uh oh!

SparkQA commented May 13, 2020

Uh oh!

iRakson commented May 14, 2020

Uh oh!

iRakson commented May 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srowen commented May 16, 2020

Uh oh!

SparkQA commented May 17, 2020

Uh oh!

iRakson commented May 18, 2020

Uh oh!

srowen commented May 18, 2020

Uh oh!

sarutak commented May 20, 2020

Uh oh!

sarutak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iRakson commented May 20, 2020

Uh oh!

SparkQA commented May 20, 2020

Uh oh!

srowen commented May 21, 2020

Uh oh!

iRakson commented May 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

iRakson commented May 12, 2020 •

edited

Loading

iRakson commented May 16, 2020 •

edited

Loading