Skip to content

[SPARK-33195][UI] Fix stages/stage UI page fails because of uri parameters encoded twice#32317

Closed
mdianjun wants to merge 1 commit into
apache:masterfrom
marsno1:fix-NullPointerException-in-tasktable
Closed

[SPARK-33195][UI] Fix stages/stage UI page fails because of uri parameters encoded twice#32317
mdianjun wants to merge 1 commit into
apache:masterfrom
marsno1:fix-NullPointerException-in-tasktable

Conversation

@mdianjun

@mdianjun mdianjun commented Apr 24, 2021

Copy link
Copy Markdown

What changes were proposed in this pull request?

The similar PR is: #29271, which is reproduced with HTTPS reverse proxy.
This PR is used to fix a similar issue in HTTP reverse proxy, described at https://issues.apache.org/jira/browse/SPARK-33195.

Why are the changes needed?

Fix a UI issue when HTTP is enabled

Does this PR introduce any user-facing change?

No.

How was this patch tested?

A new Unit test + manually test on a cluster (Added later).

@github-actions github-actions Bot added the CORE label Apr 24, 2021
@mdianjun mdianjun changed the title [SPARK-33195] Fix stages/stage UI page fails because of uri parameters encoded twice [SPARK-33195][UI] Fix stages/stage UI page fails because of uri parameters encoded twice Apr 24, 2021
@mdianjun mdianjun force-pushed the fix-NullPointerException-in-tasktable branch 2 times, most recently from 7a95234 to da616a6 Compare April 25, 2021 01:21
@HyukjinKwon

Copy link
Copy Markdown
Member

cc @sarutak and @gengliangwang FYI

@sarutak

sarutak commented Apr 26, 2021

Copy link
Copy Markdown
Member

@mdianjun

A new Unit test + manually test on a cluster (Added later).

Could you add the new test?

@mdianjun mdianjun force-pushed the fix-NullPointerException-in-tasktable branch from da616a6 to 7f7adb8 Compare April 27, 2021 08:45
@sarutak

sarutak commented Apr 28, 2021

Copy link
Copy Markdown
Member

Hi @mdianjun , could you re-trigger the test on your repository?
https://github.com/marsno1/spark/runs/2446289248?check_suite_focus=true

@sarutak

sarutak commented Apr 28, 2021

Copy link
Copy Markdown
Member

ok to test.

@SparkQA

SparkQA commented Apr 28, 2021

Copy link
Copy Markdown

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42534/

@SparkQA

SparkQA commented Apr 28, 2021

Copy link
Copy Markdown

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/42534/

@SparkQA

SparkQA commented Apr 28, 2021

Copy link
Copy Markdown

Test build #138015 has finished for PR 32317 at commit 7f7adb8.

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

isSearch = true
searchValue = uriQueryParameters.getFirst("search[value]")
} else {
searchValue = null

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this redundant right?

// refer https://datatables.net/manual/server-side.
if (uriQueryParameters.getFirst("search[value]") != null &&
uriQueryParameters.getFirst("search[value]").length > 0) {
searchValue = encodeKeyAndGetValue(uriQueryParameters, "search[value]", null)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we consider the case that searchValue is also encoded?

stageAttemptId: Int, isSearch: Boolean, totalRecords: Int): Seq[TaskData] = {
var columnNameToSort = queryParameters.getFirst("columnNameToSort")
columnNameToSort = URLDecoder.decode(columnNameToSort, "UTF-8")
columnNameToSort = URLDecoder.decode(columnNameToSort, "UTF-8")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's not obvious that the reason why columnNameToSort is decoded twice here.
Also, it may be safe for now but it's difficult to ensure the safety of decoding twice here.

@sarutak

sarutak commented May 10, 2021

Copy link
Copy Markdown
Member

@mdianjun Sorry for the late reply.
I left some comments.
I think the current solution seems a little bit dangerous.

@github-actions

Copy link
Copy Markdown

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

@github-actions github-actions Bot added the Stale label Aug 19, 2021
@github-actions github-actions Bot closed this Aug 20, 2021
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