[SPARK-30119][WEBUI] Add Pagination Support to Streaming Page #28439

iRakson · 2020-05-03T09:38:46Z

What changes were proposed in this pull request?

Pagination Support is added to all tables of streaming page in spark web UI.
For adding pagination support, existing classes from [SPARK-4598][WebUI]Task table pagination for the Stage page #7399 were used.
Earlier streaming page has two tables Active Batches and Completed Batches. Now, we will have three tables Running Batches, Waiting Batches and Completed Batches. If we have large number of waiting and running batches then keeping track in a single table is difficult. Also other pages have different table for different type type of data.
Earlier empty tables were shown. Now only non-empty tables will be shown.
Active Batches table used to show details of waiting batches followed by running batches.

Why are the changes needed?

Pagination will allow users to analyse the table in much better way. All spark web UI pages support pagination apart from streaming pages, so this will add consistency as well. Also it might fix the potential OOM errors that can arise.

Does this PR introduce any user-facing change?

Yes. Active Batches table is split into two tables Running Batches and Waiting Batches. Pagination Support is added to the all the tables. Every other functionality is unchanged.

How was this patch tested?

UT added.

Before changes:

After Changes:

iRakson · 2020-05-03T09:43:04Z

cc @srowen @dongjoon-hyun @HyukjinKwon Kindly review

srowen · 2020-05-03T14:19:59Z

Jenkins test this please

SparkQA · 2020-05-03T14:41:52Z

Test build #122231 has finished for PR 28439 at commit 1eff9f5.

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

iRakson · 2020-05-03T16:01:05Z

retest this please

srowen · 2020-05-04T14:24:45Z

Jenkins test this please

SparkQA · 2020-05-04T16:09:32Z

Test build #122270 has finished for PR 28439 at commit c07f7bf.

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

srowen · 2020-05-04T19:51:30Z

Should #28448 come before this?
I think we do want pagination here and trust your changes, but don't know this code well.
Not sure if @sarutak or @zsxwing wants to take a look.

iRakson · 2020-05-04T20:43:35Z

Should #28448 come before this?

Yeah. I think it would be better if we first clean up.

iRakson · 2020-05-22T11:50:43Z

@srowen @sarutak PR updated with latest changes in Pagination Framework.

srowen · 2020-05-22T13:48:40Z

Jenkins test this please

SparkQA · 2020-05-22T15:13:31Z

Test build #123004 has finished for PR 28439 at commit 701fb70.

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

srowen · 2020-05-24T14:28:54Z

Does this need to go in before or after #28448 ? want to make sure I understand the desired order and dependencies.

iRakson · 2020-05-24T14:44:37Z

Both are independent of each other. Either one can go first.
#28448 only cleans code, it do not introduce any new functionality or any structural changes.

iRakson · 2020-05-25T17:50:35Z

@sarutak Kindly take a look at this one.

srowen · 2020-05-28T14:21:16Z

Jenkins test this please

sarutak · 2020-05-28T15:10:02Z

@iRakson Sorry for the late reply. I'll check this weekend.

iRakson · 2020-05-28T15:35:44Z

It's ok. Please check according to your convenience. :)

SparkQA · 2020-05-28T15:44:46Z

Test build #123238 has finished for PR 28439 at commit 701fb70.

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

sarutak

@iRakson This change newly adds paged tables so I think it's better to add test cases too to test whether this change uses the pagination frame framework correctly.

sarutak · 2020-06-01T01:11:14Z

streaming/src/main/scala/org/apache/spark/streaming/ui/AllBatchesTable.scala

+      Seq(
+        ("Batch Time", true, None),
+        ("Records", true, None),
+        ("Scheduling Delay", true, Some(tooltips._1)),


Why not just put the tooltip texts as literal like other paged table?

I worked on this before working on other pages, i missed this one. I will update to literals.

sarutak · 2020-06-01T01:48:29Z

streaming/src/main/scala/org/apache/spark/streaming/ui/StreamingPage.scala

-        <div class="col-12">
-          <span id="activeBatches" class="collapse-aggregated-activeBatches collapse-table"
-                onClick="collapseTable('collapse-aggregated-activeBatches',
-                'aggregated-activeBatches')">


webui.js refers colapse-aggregated-activeBatches and aggregated-activeBatches so we need change them.

sarutak · 2020-06-01T02:19:30Z

streaming/src/main/scala/org/apache/spark/streaming/ui/AllBatchesTable.scala

+    subPath: String,
+    isRunningTable: Boolean,
+    isWaitingTable: Boolean,
+    isCompletedTable: Boolean,


isRunningTable, isWaitingTable and isCompletedTable are orthogonal so how about introducing constants which represent table types and pass one of them to the constructor?

Thank you suggesting this. I will change to constants.

sarutak · 2020-06-01T02:32:39Z

streaming/src/main/scala/org/apache/spark/streaming/ui/AllBatchesTable.scala

-      } else {
-        Nil
+    <tr>
+      <td id = {batchTimeId} isFailed = {batch.isFailed.toString}>


For attributes, it's natural not to have white spaces between = and it's operands.

sarutak · 2020-06-01T02:37:26Z

streaming/src/main/scala/org/apache/spark/streaming/ui/AllBatchesTable.scala

+          {formattedBatchTime}
+        </a>
+      </td>
+      <td> {numRecords.toString} Records </td>


It's better to keep records rather than Records to minimize appearance change.

sarutak · 2020-06-01T02:42:41Z

streaming/src/main/scala/org/apache/spark/streaming/ui/AllBatchesTable.scala

+    "table table-bordered table-sm table-striped table-head-clickable table-cell-width-limited"

-  protected def createOutputOperationProgressBar(batch: BatchUIData): Seq[Node] = {
-    <td class="progress-cell">


Why do we need to move <td> and </td> outside this method?

I removed to maintain consistency. i.e

<td> ...... </td> <td> ...... </td>

Anyway, i will use the original code.

iRakson · 2020-06-01T04:12:48Z

@sarutak Thank you for reviewing. I will update PR with test cases and suggested code changes.

iRakson · 2020-06-02T14:05:48Z

@sarutak @srowen Kindly take a look.
I added few tests for checking number of rows in paginated tables, sorting.

iRakson · 2020-06-02T14:42:51Z

retest this please

sarutak · 2020-06-02T14:56:38Z

add to whitelist.

sarutak · 2020-06-02T15:05:34Z

@iRakson From now on, Jenkins starts on pushing your commit and you can start Jenkins by saying "retest this please".

iRakson · 2020-06-02T15:33:00Z

@iRakson From now on, Jenkins starts on pushing your commit and you can start Jenkins by saying "retest this please".

@sarutak Thank you for adding me to whitelist :)

SparkQA · 2020-06-02T17:41:48Z

Test build #123439 has finished for PR 28439 at commit eb334a2.

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

SparkQA · 2020-06-02T18:14:43Z

Test build #123438 has finished for PR 28439 at commit eb334a2.

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

iRakson · 2020-06-04T09:45:47Z

retest this please

iRakson · 2020-06-04T09:46:10Z

@sarutak Gentle ping.

SparkQA · 2020-06-04T12:40:18Z

Test build #123527 has finished for PR 28439 at commit eb334a2.

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

srowen · 2020-06-06T18:05:46Z

If you're OK with it @sarutak I'll merge

sarutak · 2020-06-07T03:56:15Z

I've inspected again and it seems ok to me.

sarutak · 2020-06-07T04:04:04Z

I'll merge this time.

sarutak · 2020-06-07T04:09:32Z

@iRakson Could you update the screenshots in the description?

iRakson · 2020-06-07T04:15:45Z

@iRakson Could you update the screenshots in the description?

Updated.

iRakson · 2020-06-07T04:16:13Z

Thank You. :) @sarutak @srowen

sarutak · 2020-06-07T04:20:48Z

Merged to master.

This PR reverts #28439 due to that PR breaks QA build. Closes #28747 from sarutak/revert-SPARK-30119. Authored-by: Kousuke Saruta <[email protected]> Signed-off-by: Kousuke Saruta <[email protected]>

sarutak · 2020-06-08T01:16:31Z

@iRakson
This PR passed PR builder's test but doesn't pass QA so I've reverted.

https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-master-test-maven-hadoop-2.7-hive-1.2/lastCompletedBuild/testReport/org.apache.spark.streaming/UISeleniumSuite/attaching_and_detaching_a_Streaming_tab/
https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-master-test-maven-hadoop-2.7-hive-2.3/lastCompletedBuild/testReport/org.apache.spark.streaming/UISeleniumSuite/attaching_and_detaching_a_Streaming_tab/
https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-master-test-maven-hadoop-2.7-hive-2.3-jdk-11/lastCompletedBuild/testReport/org.apache.spark.streaming/UISeleniumSuite/attaching_and_detaching_a_Streaming_tab/
https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-master-test-maven-hadoop-3.2-hive-2.3/lastCompletedBuild/testReport/
https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-master-test-maven-hadoop-3.2-hive-2.3-jdk-11/lastCompletedBuild/testReport/org.apache.spark.streaming/UISeleniumSuite/attaching_and_detaching_a_Streaming_tab/
https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-master-test-sbt-hadoop-2.7-hive-1.2/lastCompletedBuild/testReport/org.apache.spark.streaming/UISeleniumSuite/attaching_and_detaching_a_Streaming_tab/
https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-master-test-sbt-hadoop-2.7-hive-2.3/lastCompletedBuild/testReport/

Could you confirm them?

sarutak · 2020-06-08T02:05:53Z

The suite passes on my laptop with both sbt and Maven so the suite can be flaky.

iRakson · 2020-06-08T03:24:56Z

streaming/src/test/scala/org/apache/spark/streaming/UISeleniumSuite.scala

        // Check batch tables
        val h4Text = findAll(cssSelector("h4")).map(_.text).toSeq
-        h4Text.exists(_.matches("Active Batches \\(\\d+\\)")) should be (true)
+        h4Text.exists(_.matches("Running Batches \\(\\d+\\)")) should be (true)


This is causing all the failures. I will remove these tests and raise again

### What changes were proposed in this pull request? * Pagination Support is added to all tables of streaming page in spark web UI. For adding pagination support, existing classes from apache#7399 were used. * Earlier streaming page has two tables `Active Batches` and `Completed Batches`. Now, we will have three tables `Running Batches`, `Waiting Batches` and `Completed Batches`. If we have large number of waiting and running batches then keeping track in a single table is difficult. Also other pages have different table for different type type of data. * Earlier empty tables were shown. Now only non-empty tables will be shown. `Active Batches` table used to show details of waiting batches followed by running batches. ### Why are the changes needed? Pagination will allow users to analyse the table in much better way. All spark web UI pages support pagination apart from streaming pages, so this will add consistency as well. Also it might fix the potential OOM errors that can arise. ### Does this PR introduce _any_ user-facing change? Yes. `Active Batches` table is split into two tables `Running Batches` and `Waiting Batches`. Pagination Support is added to the all the tables. Every other functionality is unchanged. ### How was this patch tested? Manually. Before changes: <img width="1667" alt="Screenshot 2020-05-03 at 7 07 14 PM" src="https://user-images.githubusercontent.com/15366835/80915680-8fb44b80-8d71-11ea-9957-c4a3769b8b67.png"> After Changes: <img width="1669" alt="Screenshot 2020-05-03 at 6 51 22 PM" src="https://user-images.githubusercontent.com/15366835/80915694-a9ee2980-8d71-11ea-8fc5-246413a4951d.png"> Closes apache#28439 from iRakson/streamingPagination. Authored-by: iRakson <[email protected]> Signed-off-by: Kousuke Saruta <[email protected]>

### What changes were proposed in this pull request? #28747 reverted #28439 due to some flaky test case. This PR fixes the flaky test and adds pagination support. ### Why are the changes needed? To support pagination for streaming tab ### Does this PR introduce _any_ user-facing change? Yes, Now streaming tab tables will be paginated. ### How was this patch tested? Manually. Closes #28748 from iRakson/fixstreamingpagination. Authored-by: iRakson <[email protected]> Signed-off-by: Sean Owen <[email protected]>

probot-autolabeler bot added DSTREAM WEB UI labels May 3, 2020

iRakson mentioned this pull request May 18, 2020

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

Closed

iRakson added 3 commits May 22, 2020 16:32

[SPARK-30119] Add Pagination Support to Streaming Page

65d4132

fix

f534bc9

rebase and update

701fb70

iRakson force-pushed the streamingPagination branch from c07f7bf to 701fb70 Compare May 22, 2020 11:43

sarutak reviewed Jun 1, 2020

View reviewed changes

fix

eb334a2

probot-autolabeler bot added the CORE label Jun 2, 2020

iRakson requested a review from sarutak June 2, 2020 14:43

sarutak closed this in e9337f5 Jun 7, 2020

GulajavaMinistudio mentioned this pull request Jun 7, 2020

Choose two branches to see what’s changed or to start a new pull request.[SPARK-30119][WEBUI] Add Pagination Support to Streaming Page GulajavaMinistudio/spark#778

Merged

sarutak mentioned this pull request Jun 8, 2020

Revert "[SPARK-30119][WEBUI] Add Pagination Support to Streaming Page" #28747

Closed

iRakson commented Jun 8, 2020

View reviewed changes

iRakson mentioned this pull request Jun 8, 2020

[SPARK-30119][WEBUI]Support pagination for streaming tab #28748

Closed

[SPARK-30119][WEBUI] Add Pagination Support to Streaming Page #28439

[SPARK-30119][WEBUI] Add Pagination Support to Streaming Page #28439

Uh oh!

Conversation

iRakson commented May 3, 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 3, 2020

Uh oh!

srowen commented May 3, 2020

Uh oh!

SparkQA commented May 3, 2020

Uh oh!

iRakson commented May 3, 2020

Uh oh!

srowen commented May 4, 2020

Uh oh!

SparkQA commented May 4, 2020

Uh oh!

srowen commented May 4, 2020

Uh oh!

iRakson commented May 4, 2020

Uh oh!

iRakson commented May 22, 2020

Uh oh!

srowen commented May 22, 2020

Uh oh!

SparkQA commented May 22, 2020

Uh oh!

srowen commented May 24, 2020

Uh oh!

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

Uh oh!

Uh oh!

iRakson commented May 25, 2020

Uh oh!

srowen commented May 28, 2020

Uh oh!

sarutak commented May 28, 2020

Uh oh!

iRakson commented May 28, 2020

Uh oh!

SparkQA commented May 28, 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!

iRakson commented Jun 1, 2020

Uh oh!

iRakson commented Jun 2, 2020

Uh oh!

iRakson commented Jun 2, 2020

Uh oh!

sarutak commented Jun 2, 2020

Uh oh!

sarutak commented Jun 2, 2020

Uh oh!

iRakson commented Jun 2, 2020

iRakson commented May 3, 2020 •

edited

Loading

iRakson commented May 24, 2020 •

edited

Loading

sarutak commented Jun 8, 2020 •

edited

Loading