[SPARK-9724] [WEB UI] Avoid unnecessary redirects in the Spark Web UI.

[rohitagarwal003]

No description provided.

[JoshRosen]

Jenkins, this is ok to test.

[JoshRosen]

I don't know who is the best person to review this. @vanzin and @imran, maybe, given that you guys have worked with Jetty? @zsxwing, perhaps, given your recent UI work?

[vanzin]

I know you're copying & pasting the code from above, but I wonder how is this any different than just always returning basePath + relativePath.

[vanzin]

In fact, the conditional makes sense in the original method, because of the stripSuffix call, but does not make sense here - nor does the copied scaladoc about "avoid returning an empty path".

[rohitagarwal003]

Agreed on both counts. Updated.

[vanzin]

Makes sense after looking at how WebUI.attachPage works.

[SparkQA]

Test build #40129 has finished for PR 8014 at commit b257844.

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

[SparkQA]

Test build #40143 has finished for PR 8014 at commit 8a977cd.

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

[rohitagarwal003]

Can this be merged now @vanzin?

[srowen]

Both of these methods seems superfluous. They're trivial and each called once. I don't see why basePath == "" is treated separately, like, why the trailing slash wouldn't be stripped. In fact, there's only one way a call sets basePath to anything but "" and it's the /jobs servlet below - not clear why only that is special cased?

Anyway I'd suggest removing the methods and removing the special-casing -- it's just the combination of two strings, with a trailing / stripped in one case.

[rohitagarwal003]

basePath == "" is treated separately because there is a case when basePath = "" and relativePath = "/". In that case, we would return an empty string if we don't treat basePath == "" separately.

Here are the various cases:

                                          + /
                                          + /json

  /history/application_1438648766072_0009 + /jobs
  /history/application_1438648766072_0009 + /jobs/json
  /history/application_1438648766072_0009 + /jobs/job
  /history/application_1438648766072_0009 + /jobs/job/json
  /history/application_1438648766072_0009 + /stages
  /history/application_1438648766072_0009 + /stages/json
  /history/application_1438648766072_0009 + /stages/stage
  /history/application_1438648766072_0009 + /stages/stage/json
  /history/application_1438648766072_0009 + /stages/pool
  /history/application_1438648766072_0009 + /stages/pool/json
  /history/application_1438648766072_0009 + /storage
  /history/application_1438648766072_0009 + /storage/json
  /history/application_1438648766072_0009 + /storage/rdd
  /history/application_1438648766072_0009 + /storage/rdd/json
  /history/application_1438648766072_0009 + /environment
  /history/application_1438648766072_0009 + /environment/json
  /history/application_1438648766072_0009 + /executors
  /history/application_1438648766072_0009 + /executors/json
* /history/application_1438648766072_0009 + /jobs/
  /history/application_1438648766072_0009 + /
*                                         + /stages/
                                          + /stages/stage/kill

The ones with asterisk are for attachPrefixForRedirect, the other ones are for attachPrefix.

Earlier attachPrefix was called from two places, but now from one of these places attachPrefixForRedirect is being called. Let me know if you want me to inline these.

[srowen]

Yes but I'm curious why basePath == "" plus relativePath == "foo/" should retain its trailing slash but not when basePath is non-empty. Is it really basePath =="" && relativePath =="/" that should be handled specially?

I do think it would be better to inline them. I am not sure why the suffix-stripping logic exists in the first place but that may be too far afield.

[rohitagarwal003]

I am not sure why the suffix-stripping logic exists in the first place but that may be too far afield.

That's because contextHandler.setContextPath() which uses the result of attachPrefix, throws an IllegalArgumentException when the path ends with /.

Yes but I'm curious why basePath == "" plus relativePath == "foo/" should retain its trailing slash but not when basePath is non-empty. Is it really basePath =="" && relativePath =="/" that should be handled specially?

Yes, it seems like only basePath =="" && relativePath =="/" should be handled specially. In fact, basePath == "" plus relativePath == "foo/" would result in the IllegalArgumentException mentioned above. I guess we just never encountered that case.

[rohitagarwal003]

@srowen I have inlined the methods and fixed the if condition for calculating prefixedPath.

[SparkQA]

Test build #40364 timed out for PR 8014 at commit a7af5ff after a configured wait of 175m.

[SparkQA]

Test build #1445 has finished for PR 8014 at commit a7af5ff.

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

[rohitagarwal003]

Can we retest this? The test failures seem unrelated to my changes.

[zsxwing]

retest this please.

[SparkQA]

Test build #40629 has finished for PR 8014 at commit a7af5ff.

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

[rohitagarwal003]

@vanzin, @srowen Does this look good now?

[vanzin]

LGTM.

[vanzin]

Merged to master. Thanks!