[SPARK-24490][WebUI] Use WebUI.addStaticHandler in web UIs #21510
Conversation
|
Test build #91536 has finished for PR 21510 at commit
|
|
LGTM |
| @@ -101,12 +101,12 @@ private[spark] abstract class WebUI( | |||
| } | |||
|
|
|||
| /** | |||
| * Add a handler for static content. | |||
| * Adds a handler for static content. | |||
There was a problem hiding this comment.
In this file, s is not added.
It would be good to add s to comments in all of the methods if we add s.
There was a problem hiding this comment.
@jaceklaskowski please address this super tiny issue.
|
LGTM with one minor comment. |
|
@kiszk @jerryshao @srowen Added |
| attachHandler(JettyUtils.createStaticHandler(resourceBase, path)) | ||
| } | ||
|
|
||
| /** | ||
| * Remove a static content handler. | ||
| * Removes a static content handler. | ||
| * | ||
| * @param path Path in UI to unmount. | ||
| */ | ||
| def removeStaticHandler(path: String): Unit = { |
There was a problem hiding this comment.
This method name is misleading. It will remove any handler, not just "static" handlers... and since you're touching this code...
There was a problem hiding this comment.
OK...since @vanzin requested I'm gonna make all the other changes while at it :)
There was a problem hiding this comment.
I think this could still be addressed? I think it's fine to rename the method as it's all private to Spark.
|
Test build #91662 has finished for PR 21510 at commit
|
|
retest this please. |
|
Test build #91736 has finished for PR 21510 at commit
|
|
Test build #91899 has finished for PR 21510 at commit
|
|
Test build #91902 has finished for PR 21510 at commit
|
|
Merging to master. |
WebUIdefinesaddStaticHandlerthat web UIs don't use (and simply introduce duplication). Let's clean them up and remove duplications.Local build and waiting for Jenkins