Skip to content

[SPARK-28677][WEBUI] "Select All" checkbox in StagePage doesn't work properly#25397

Closed
sarutak wants to merge 2 commits intoapache:masterfrom
sarutak:fix-stagepage.js
Closed

[SPARK-28677][WEBUI] "Select All" checkbox in StagePage doesn't work properly#25397
sarutak wants to merge 2 commits intoapache:masterfrom
sarutak:fix-stagepage.js

Conversation

@sarutak
Copy link
Member

@sarutak sarutak commented Aug 9, 2019

What changes were proposed in this pull request?

In StagePage, only the first optional column (Scheduler Delay, in this case) appears even though "Select All" checkbox is checked.

Screenshot from 2019-08-09 18-46-05

The cause is that wrong method is used to manipulate multiple columns. columns should have been used but column was used.
I've fixed this issue by replacing the column with columns.

How was this patch tested?

Confirmed behavior of the check-box.

Screenshot from 2019-08-09 18-54-33

@sarutak
Copy link
Member Author

sarutak commented Aug 9, 2019

CC: @pgandhi999 and @tgravescs

@SparkQA
Copy link

SparkQA commented Aug 9, 2019

Test build #108877 has finished for PR 25397 at commit 27d2991.

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

@sarutak
Copy link
Member Author

sarutak commented Aug 9, 2019

retest this please.

@SparkQA
Copy link

SparkQA commented Aug 9, 2019

Test build #108879 has finished for PR 25397 at commit 27d2991.

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

@pgandhi999
Copy link

@sarutak Thank you for catching the bug. Interestingly, I am only able to reproduce the bug in Spark 3.0, it works fine in Spark 2.3 and 2.4. Are you also able to reproduce the bug in 2.3 and 2.4?

Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

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

OK by me if it's a clean UI fix.

Copy link

@pgandhi999 pgandhi999 Aug 9, 2019

Choose a reason for hiding this comment

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

NIT: Could you rename the variable to something like allColumns to have more clarity?

@sarutak
Copy link
Member Author

sarutak commented Aug 9, 2019

@pgandhi999

Thank you for catching the bug. Interestingly, I am only able to reproduce the bug in Spark 3.0, it works fine in Spark 2.3 and 2.4. Are you also able to reproduce the bug in 2.3 and 2.4?

2.3 and 2.4 use additional-metrics.js rather than stagepage.js for the checkboxes.
I think that's why only 3.0 has this issue.

@pgandhi999
Copy link

2.3 and 2.4 use additional-metrics.js rather than stagepage.js for the checkboxes.
I think that's why only 3.0 has this issue.

Ah, that makes sense. Thank you.

@pgandhi999
Copy link

LGTM +1

@tgravescs
Copy link
Contributor

"2.3 and 2.4 use additional-metrics.js rather than stagepage.js for the checkboxes.
I think that's why only 3.0 has this issue."

Can you please explain this in more details so we have it documented what really book it? Please also update the description to say what you change and ideally why it was broken.

@sarutak
Copy link
Member Author

sarutak commented Aug 9, 2019

Can you please explain this in more details so we have it documented what really book it? Please

stagepage.js is not present in 2.3 and 2.4. This bug has been brought by stagepage.js which is merged into master.
The cause is that wrong method is used to manipulate multiple columns. columns should have been used but column was used.

@SparkQA
Copy link

SparkQA commented Aug 9, 2019

Test build #108882 has finished for PR 25397 at commit 2f7f558.

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

Copy link
Contributor

@tgravescs tgravescs left a comment

Choose a reason for hiding this comment

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

changes look good. thanks @sarutak

@srowen
Copy link
Member

srowen commented Aug 10, 2019

Merged to master

@srowen srowen closed this in dd5599e Aug 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

Comments