Skip to content

Conversation

@nblintao
Copy link
Contributor

@nblintao nblintao commented Jun 11, 2016

What changes were proposed in this pull request?

This patch adds pagination support for the Job Tables in the Jobs tab. Pagination is provided for all of the three Job Tables (active, completed, and failed). Interactions (jumping, sorting, and setting page size) for paged tables are also included.

The diff didn't keep track of some lines based on the original ones. The function makeRowof the original AllJobsPage.scala is reused. They are separated at the beginning of the function jobRow (L427-439) and the function row(L594-618) in the new AllJobsPage.scala.

How was this patch tested?

Tested manually by using checking the Web UI after completing and failing hundreds of jobs.
Generate completed jobs by:

val d = sc.parallelize(Array(1,2,3,4,5))
for(i <- 1 to 255){ var b = d.collect() }

Generate failed jobs by calling the following code multiple times:

var b = d.map(_/0).collect()

Interactions like jumping, sorting, and setting page size are all tested.

This shows the pagination for completed jobs:
paginate success jobs

This shows the sorting works in job tables:
sorting

This shows the pagination for failed jobs and the effect of jumping and setting page size:
paginate failed jobs

@andrewor14
Copy link
Contributor

add to whitelist

r
}

def slicedJobIds: Set[Int] = _slicedJobIds
Copy link
Contributor

Choose a reason for hiding this comment

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

used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, it's deprecated. I will remove it. Thanks.

@andrewor14
Copy link
Contributor

@nblintao this looks pretty good. There were a few methods that don't seem to be used anywhere. Am I missing something?

@SparkQA
Copy link

SparkQA commented Jun 14, 2016

Test build #60444 has finished for PR 13620 at commit 4db0e09.

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

@SparkQA
Copy link

SparkQA commented Jun 14, 2016

Test build #60469 has finished for PR 13620 at commit 6682e86.

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

@SparkQA
Copy link

SparkQA commented Jun 15, 2016

Test build #60543 has finished for PR 13620 at commit 41bb189.

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

@ajbozarth
Copy link
Member

ajbozarth commented Jun 15, 2016

This looks great, I checked out the code and it looks good locally on my machine as well. I've read through the code and it looks good with the fixes @andrewor14 suggested so LGTM (see below)

@andrewor14
Copy link
Contributor

also cc @zsxwing

@ajbozarth
Copy link
Member

Just opened this in the history server to double check the bug I found on your stages pr and discovered that this is completely non-functional on the history server, so I'm striking my earlier LGTM

@nblintao
Copy link
Contributor Author

@ajbozarth Thanks for your checking and reporting. I didn't notice that the UI for history servers are different and just changed the AllJobsPage.

I will try to check the page for history servers. But do you think it would be better if we use another PR to solve this (adding the pagination support for the jobs page in history servers)?

@ajbozarth
Copy link
Member

They actually share the core majority of their code so your pagination is already there, it just can't change pages or change the number of shown jobs. I think its has to do with how your calling and parsing the http requests since I didn't see any parameters in the url when I would try to change pages

@nblintao
Copy link
Contributor Author

nblintao commented Jun 17, 2016

@ajbozarth Got it, that's a really fatal bug. I will fix it in this PR. Thanks again!

@nblintao
Copy link
Contributor Author

This PR has known bugs about linking. I have fixed the related bugs in #13708. In order to avoid conflicts, I will not update this PR until #13708 is merged.
So, please do NOT review or test this PR for now. It would be nice if you could help review or test #13708. Thanks!

ghost pushed a commit to dbtsai/spark that referenced this pull request Jul 6, 2016
## What changes were proposed in this pull request?

This patch adds pagination support for the Stage Tables in the Stage tab. Pagination is provided for all of the four Job Tables (active, pending, completed, and failed). Besides, the paged stage tables are also used in JobPage (the detail page for one job) and PoolPage.

Interactions (jumping, sorting, and setting page size) for paged tables are also included.

## How was this patch tested?

Tested manually by using checking the Web UI after completing and failing hundreds of jobs.  Same as the testings for [Paginate Job Table in Jobs tab](apache#13620).

This shows the pagination for completed stages:
![paged stage table](https://cloud.githubusercontent.com/assets/5558370/16125696/5804e35e-3427-11e6-8923-5c5948982648.png)

Author: Tao Lin <[email protected]>

Closes apache#13708 from nblintao/stageTable.
@SparkQA
Copy link

SparkQA commented Jul 7, 2016

Test build #61910 has finished for PR 13620 at commit 649bb19.

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

@nblintao
Copy link
Contributor Author

nblintao commented Jul 7, 2016

I believe this commit has resolved the bugs reported by @ajbozarth. It looks well on history server pages now, and it could keep the status of other tables while changing one.
Could you please help test or review it if you are available? Thanks! @andrewor14 @zsxwing @ajbozarth

@ajbozarth
Copy link
Member

@nblintao I just finished testing your changes and it looks good. I did find a odd behavior though, but I think it's intentional in the original pagination code. If you make the table large enough to fit all the jobs the pagination toolbar disappears. Given you can just click the back button to fix it I don't think it's an issue to be fixed, but it is a bit annoying. Another fyi is that only ~1000 jobs are shown still, but I'm assuming the pagination doesn't fix the reasoning behind only showing the most recent 1000 jobs.

tl;dr LGTM

jobs: Seq[JobUIData]): Seq[Node] = {
val allParameters = request.getParameterMap.asScala.toMap
val parameterOtherTable = allParameters.filterNot(_._1.startsWith(jobTag))
.map(para => para._1 + "=" + para._2(0))
Copy link
Contributor

Choose a reason for hiding this comment

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

What does parameterOtherTable do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It keeps the status of other tables on the page. See #13708 (comment)

@yhuai
Copy link
Contributor

yhuai commented Jul 24, 2016

@nblintao Can you comment on your PR to explain which parts are new code and which parts are based on existing code (comments like https://github.com/apache/spark/pull/13620/files#r71997586)?

override def row(jobTableRow: JobTableRowData): Seq[Node] = {
val job = jobTableRow.jobData

<tr id={"job-" + job.jobId}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

@nblintao
Copy link
Contributor Author

nblintao commented Jul 25, 2016

@yhuai I updated the description of the PR:

The diff didn't keep track of some lines based on the original ones. The function makeRowof the original AllJobsPage.scala is reused. They are separated at the beginning of the function jobRow (L427-439) and the function row(L594-618) in the new AllJobsPage.scala.

And thanks for your review!

@zsxwing
Copy link
Member

zsxwing commented Jul 25, 2016

retest this please

@zsxwing
Copy link
Member

zsxwing commented Jul 25, 2016

LGTM. Let's run the test again since this is a pretty old PR. Thanks!

@SparkQA
Copy link

SparkQA commented Jul 26, 2016

Test build #62846 has finished for PR 13620 at commit 649bb19.

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

@zsxwing
Copy link
Member

zsxwing commented Jul 26, 2016

Merging to master.

@asfgit asfgit closed this in db36e1e Jul 26, 2016
@nblintao
Copy link
Contributor Author

Great! Thanks, @andrewor14 @ajbozarth @yhuai @zsxwing.

asfgit pushed a commit that referenced this pull request Dec 24, 2016
## What changes were proposed in this pull request?

This issue was reported by wangyum.

In the AllJobsPage, JobPage and StagePage, the description length was limited before like as follows.

![ui-2 0 0](https://cloud.githubusercontent.com/assets/4736016/21319673/8b225246-c651-11e6-9041-4fcdd04f4dec.gif)

But recently, the limitation seems to have been accidentally removed.

![ui-2 1 0](https://cloud.githubusercontent.com/assets/4736016/21319825/104779f6-c652-11e6-8bfa-dfd800396352.gif)

The cause is that some tables are no longer `sortable` class although they were, and `sortable` class does not only mark tables as sortable but also limited the width of their child `td` elements.
The reason why now some tables are not `sortable` class is because another sortable mechanism was introduced by #13620 and #13708 with pagination feature.

To fix this issue, I've introduced new class `table-cell-width-limited` which limits the description cell width and the description is like what it was.

<img width="1260" alt="2016-12-20 1 00 34" src="https://cloud.githubusercontent.com/assets/4736016/21320478/89141c7a-c654-11e6-8494-f8f91325980b.png">

## How was this patch tested?

Tested manually with my browser.

Author: Kousuke Saruta <[email protected]>

Closes #16338 from sarutak/SPARK-18837.
asfgit pushed a commit that referenced this pull request Dec 24, 2016
## What changes were proposed in this pull request?

This issue was reported by wangyum.

In the AllJobsPage, JobPage and StagePage, the description length was limited before like as follows.

![ui-2 0 0](https://cloud.githubusercontent.com/assets/4736016/21319673/8b225246-c651-11e6-9041-4fcdd04f4dec.gif)

But recently, the limitation seems to have been accidentally removed.

![ui-2 1 0](https://cloud.githubusercontent.com/assets/4736016/21319825/104779f6-c652-11e6-8bfa-dfd800396352.gif)

The cause is that some tables are no longer `sortable` class although they were, and `sortable` class does not only mark tables as sortable but also limited the width of their child `td` elements.
The reason why now some tables are not `sortable` class is because another sortable mechanism was introduced by #13620 and #13708 with pagination feature.

To fix this issue, I've introduced new class `table-cell-width-limited` which limits the description cell width and the description is like what it was.

<img width="1260" alt="2016-12-20 1 00 34" src="https://cloud.githubusercontent.com/assets/4736016/21320478/89141c7a-c654-11e6-8494-f8f91325980b.png">

## How was this patch tested?

Tested manually with my browser.

Author: Kousuke Saruta <[email protected]>

Closes #16338 from sarutak/SPARK-18837.

(cherry picked from commit f2ceb2a)
Signed-off-by: Sean Owen <[email protected]>
cmonkey pushed a commit to cmonkey/spark that referenced this pull request Dec 26, 2016
## What changes were proposed in this pull request?

This issue was reported by wangyum.

In the AllJobsPage, JobPage and StagePage, the description length was limited before like as follows.

![ui-2 0 0](https://cloud.githubusercontent.com/assets/4736016/21319673/8b225246-c651-11e6-9041-4fcdd04f4dec.gif)

But recently, the limitation seems to have been accidentally removed.

![ui-2 1 0](https://cloud.githubusercontent.com/assets/4736016/21319825/104779f6-c652-11e6-8bfa-dfd800396352.gif)

The cause is that some tables are no longer `sortable` class although they were, and `sortable` class does not only mark tables as sortable but also limited the width of their child `td` elements.
The reason why now some tables are not `sortable` class is because another sortable mechanism was introduced by apache#13620 and apache#13708 with pagination feature.

To fix this issue, I've introduced new class `table-cell-width-limited` which limits the description cell width and the description is like what it was.

<img width="1260" alt="2016-12-20 1 00 34" src="https://cloud.githubusercontent.com/assets/4736016/21320478/89141c7a-c654-11e6-8494-f8f91325980b.png">

## How was this patch tested?

Tested manually with my browser.

Author: Kousuke Saruta <[email protected]>

Closes apache#16338 from sarutak/SPARK-18837.
uzadude pushed a commit to uzadude/spark that referenced this pull request Jan 27, 2017
## What changes were proposed in this pull request?

This issue was reported by wangyum.

In the AllJobsPage, JobPage and StagePage, the description length was limited before like as follows.

![ui-2 0 0](https://cloud.githubusercontent.com/assets/4736016/21319673/8b225246-c651-11e6-9041-4fcdd04f4dec.gif)

But recently, the limitation seems to have been accidentally removed.

![ui-2 1 0](https://cloud.githubusercontent.com/assets/4736016/21319825/104779f6-c652-11e6-8bfa-dfd800396352.gif)

The cause is that some tables are no longer `sortable` class although they were, and `sortable` class does not only mark tables as sortable but also limited the width of their child `td` elements.
The reason why now some tables are not `sortable` class is because another sortable mechanism was introduced by apache#13620 and apache#13708 with pagination feature.

To fix this issue, I've introduced new class `table-cell-width-limited` which limits the description cell width and the description is like what it was.

<img width="1260" alt="2016-12-20 1 00 34" src="https://cloud.githubusercontent.com/assets/4736016/21320478/89141c7a-c654-11e6-8494-f8f91325980b.png">

## How was this patch tested?

Tested manually with my browser.

Author: Kousuke Saruta <[email protected]>

Closes apache#16338 from sarutak/SPARK-18837.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants