Skip to content

Conversation

@iRakson
Copy link
Contributor

@iRakson iRakson commented May 9, 2020

What changes were proposed in this pull request?

Add Pagination Support for structured streaming page. Now both tables Active Queries and Completed Queries will have pagination.
To implement pagination, pagination framework from #7399 is used.

  • Also tables will only be shown if there is at least one entry in the table.

Why are the changes needed?

  • This will help users in analysing their structured streaming queries in much better way.
  • Other Web UI pages support pagination in their table. So this will make web UI more consistent across pages.
  • This can prevent potential OOM errors.

Does this PR introduce any user-facing change?

Yes. Both tables will support pagination.

How was this patch tested?

Before Changes:
Screenshot 2020-05-28 at 12 01 49 AM

After Changes:
Screenshot 2020-05-28 at 12 22 28 AM

@iRakson
Copy link
Contributor Author

iRakson commented May 9, 2020

cc @srowen @zsxwing @sarutak @uncleGen Kindly Review

@HyukjinKwon
Copy link
Member

ok to test

@HyukjinKwon
Copy link
Member

HyukjinKwon commented May 10, 2020

cc @gengliangwang and @HeartSaVioR as well

@SparkQA
Copy link

SparkQA commented May 10, 2020

Test build #122469 has finished for PR 28485 at commit df61b7c.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class StreamingQueryPagedTable(
  • case class StructuredStreamingRow(
  • class StreamingQueryDataSource(uiData: Seq[StreamingQueryUIData], sortColumn: String, desc: Boolean,

@iRakson
Copy link
Contributor Author

iRakson commented May 10, 2020

retest this please

@srowen
Copy link
Member

srowen commented May 10, 2020

Jenkins retest this please

@SparkQA
Copy link

SparkQA commented May 10, 2020

Test build #122479 has finished for PR 28485 at commit df61b7c.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class StreamingQueryPagedTable(
  • case class StructuredStreamingRow(
  • class StreamingQueryDataSource(uiData: Seq[StreamingQueryUIData], sortColumn: String, desc: Boolean,

@uncleGen
Copy link
Contributor

uncleGen commented May 11, 2020

Seems like an unrelated and flaky failure and will fixed in #28391

@uncleGen
Copy link
Contributor

Jenkins retest this please

@SparkQA
Copy link

SparkQA commented May 11, 2020

Test build #122485 has finished for PR 28485 at commit df61b7c.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class StreamingQueryPagedTable(
  • case class StructuredStreamingRow(
  • class StreamingQueryDataSource(uiData: Seq[StreamingQueryUIData], sortColumn: String, desc: Boolean,

Copy link
Contributor

@uncleGen uncleGen left a comment

Choose a reason for hiding this comment

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

Bunch of codes seem to be copied from codebase, it might be better to find the way how to reuse them.

Comment on lines 162 to 166
s"&$pageNumberFormField=$page" +
s"&$tableTag.sort=$encodedSortColumn" +
s"&$tableTag.desc=$sortDesc" +
s"&$pageSizeFormField=$pageSize" +
s"#$tableTag"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: indent

}

eventually(timeout(30.seconds), interval(100.milliseconds)) {
eventually(timeout(300.seconds), interval(100.milliseconds)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was modified accidentally, I will update this.

@SparkQA
Copy link

SparkQA commented May 11, 2020

Test build #122493 has finished for PR 28485 at commit 2486177.

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

List("Name", "Status", "Id", "Run ID", "Start Time", "Duration", "Avg Input /sec",
"Avg Process /sec", "Lastest Batch")

val arrow = 0x25BE.toChar
Copy link
Member

Choose a reason for hiding this comment

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

nit: Why not just simply use a literal like '\u25BE' ?

@uncleGen
Copy link
Contributor

LGTM. Pending on #28512 accepted and then update this PR.

srowen pushed a commit that referenced this pull request May 21, 2020
### 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](#28485 (review)).

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.

Closes #28512 from iRakson/refactorPaginationFramework.

Authored-by: iRakson <[email protected]>
Signed-off-by: Sean Owen <[email protected]>
@iRakson
Copy link
Contributor Author

iRakson commented May 21, 2020

@srowen @sarutak @uncleGen This PR has been updated.

@SparkQA
Copy link

SparkQA commented May 22, 2020

Test build #122941 has finished for PR 28485 at commit 088c551.

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

@srowen
Copy link
Member

srowen commented May 22, 2020

Jenkins retest this please

@SparkQA
Copy link

SparkQA commented May 22, 2020

Test build #123008 has finished for PR 28485 at commit 088c551.

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

@sarutak sarutak closed this in fbb3144 May 23, 2020
@sarutak
Copy link
Member

sarutak commented May 23, 2020

I've merged to master.
Thanks @iRakson !

@iRakson
Copy link
Contributor Author

iRakson commented May 23, 2020

Thank You 😊 @sarutak @srowen @uncleGen

("Run ID", true, None),
("Start Time", true, None),
("Duration", true, None),
("Avg Input /sec", true, None),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@srowen @uncleGen @sarutak Should we add tooltips for these columns.

@gatorsmile
Copy link
Member

. I will add snapshots soon.

@iRakson could you update the PR description with the screenshot.

@iRakson
Copy link
Contributor Author

iRakson commented May 27, 2020

@iRakson could you update the PR description with the screenshot.

Updated.

srowen pushed a commit that referenced this pull request Jun 14, 2020
…us column sortable

### What changes were proposed in this pull request?
In #28485 pagination support for tables of Structured Streaming Tab was added.
It missed 2 things:
* For sorting duration column, `String` was used which sometimes gives wrong results(consider `"3 ms"` and `"12 ms"`). Now we first sort the duration column and then convert it to readable String
* Status column was not made sortable.

### Why are the changes needed?
To fix the wrong result for sorting and making Status column sortable.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
After changes:
<img width="1677" alt="Screenshot 2020-06-08 at 2 18 48 PM" src="https://user-images.githubusercontent.com/15366835/84010992-153fa280-a993-11ea-9846-bf176f2ec5d7.png">

Closes #28752 from iRakson/ssTests.

Authored-by: iRakson <[email protected]>
Signed-off-by: Sean Owen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants