Skip to content

Conversation

@iRakson
Copy link
Contributor

@iRakson iRakson commented May 4, 2020

What changes were proposed in this pull request?

Pagination code across pages needs to be cleaned.
I have tried to clear out these things :

  • Unused methods
  • Unused method arguments
  • remove redundant if expressions
  • fix indentation

Why are the changes needed?

This fix will make code more readable and remove unnecessary methods and variables.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Manually

@iRakson iRakson changed the title Clean Pagination Code for all pages [SPARK-31638][WEBUI]Clean Pagination code for all web May 4, 2020
@iRakson iRakson changed the title [SPARK-31638][WEBUI]Clean Pagination code for all web [SPARK-31638][WEBUI]Clean Pagination code for all webUI pages May 4, 2020
@iRakson
Copy link
Contributor Author

iRakson commented May 4, 2020

cc @srowen @dongjoon-hyun @HyukjinKwon Kindly Review

@srowen
Copy link
Member

srowen commented May 4, 2020

Jenkins test this please

@srowen
Copy link
Member

srowen commented May 4, 2020

Likewise I don't know this code well, but code cleanup is a good thing.
@sarutak or @zsxwing might know more.
I'm mostly trying to think if this can break anything, like replaying events from a history server with an old format or something. Not even sure that makes sense.

@SparkQA
Copy link

SparkQA commented May 4, 2020

Test build #122289 has finished for PR 28448 at commit 8097037.

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

@sarutak
Copy link
Member

sarutak commented May 6, 2020

I'll check within a week.

store,
data,
basePath,
currentTime,
Copy link
Member

Choose a reason for hiding this comment

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

If we can remove currentTime here, can we also remove currentTime from JobPagedTable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah. It can be removed. Thanks.


override val dataSource: TaskDataSource = new TaskDataSource(
stage,
currentTime,
Copy link
Member

Choose a reason for hiding this comment

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

Similar to the comment for AllJobsPage, can we remove currentTime from TaskPagedTable?

</span>
}
}
<span data-toggle="tooltip" data-placement="top" title={tooltip.getOrElse("")}>
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch. Tooltips with zero-length titles are never displayed.

@iRakson iRakson requested a review from sarutak May 11, 2020 07:20
@iRakson
Copy link
Contributor Author

iRakson commented May 11, 2020

There is a bunch of code which is being copied whenever we implement pagination as pointed out here.

I am thinking of making some changes to PagedTable implementation. Should I introduce them in another PR as this PR deals mostly with removal of dead code, indentation, etc. Also it already has huge amount of changes? @srowen @sarutak

@srowen
Copy link
Member

srowen commented May 12, 2020

Up to your judgment about whether they can be logically broken up into separate changes that are easier to reason about, or go together.

@iRakson iRakson force-pushed the refactorPagination branch from 9fdec71 to d379a86 Compare May 23, 2020 14:33
@iRakson
Copy link
Contributor Author

iRakson commented May 23, 2020

@srowen @sarutak PR has been updated. Kindly take a look at this one.

@iRakson
Copy link
Contributor Author

iRakson commented May 23, 2020

retest this please

@sarutak
Copy link
Member

sarutak commented May 23, 2020

retest this please.

@SparkQA
Copy link

SparkQA commented May 24, 2020

Test build #123036 has finished for PR 28448 at commit 65007e7.

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

@srowen
Copy link
Member

srowen commented May 27, 2020

Merged to master

@srowen srowen closed this in 765105b May 27, 2020
@iRakson
Copy link
Contributor Author

iRakson commented May 27, 2020

Thank You @srowen @sarutak

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.

4 participants