[GitHub] spark pull request #21510: [SPARK-24490][WebUI] Use WebUI.addStaticHandler i...
Github user asfgit closed the pull request at: https://github.com/apache/spark/pull/21510 --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21510: [SPARK-24490][WebUI] Use WebUI.addStaticHandler i...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/21510#discussion_r195074091
--- Diff: core/src/main/scala/org/apache/spark/ui/WebUI.scala ---
@@ -88,41 +90,41 @@ private[spark] abstract class WebUI(
handlers += renderHandler
}
- /** Attach a handler to this UI. */
+ /** Attaches a handler to this UI. */
def attachHandler(handler: ServletContextHandler) {
handlers += handler
serverInfo.foreach(_.addHandler(handler))
}
- /** Detach a handler from this UI. */
+ /** Detaches a handler from this UI. */
def detachHandler(handler: ServletContextHandler) {
handlers -= handler
serverInfo.foreach(_.removeHandler(handler))
}
/**
- * Add a handler for static content.
+ * Adds a handler for static content.
*
* @param resourceBase Root of where to find resources to serve.
* @param path Path in UI where to mount the resources.
*/
- def addStaticHandler(resourceBase: String, path: String): Unit = {
+ def addStaticHandler(resourceBase: String, path: String = "/static"):
Unit = {
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 = {
--- End diff --
I think this could still be addressed? I think it's fine to rename the
method as it's all private to Spark.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21510: [SPARK-24490][WebUI] Use WebUI.addStaticHandler i...
Github user jaceklaskowski commented on a diff in the pull request:
https://github.com/apache/spark/pull/21510#discussion_r194632125
--- Diff: core/src/main/scala/org/apache/spark/ui/WebUI.scala ---
@@ -88,41 +90,41 @@ private[spark] abstract class WebUI(
handlers += renderHandler
}
- /** Attach a handler to this UI. */
+ /** Attaches a handler to this UI. */
def attachHandler(handler: ServletContextHandler) {
handlers += handler
serverInfo.foreach(_.addHandler(handler))
}
- /** Detach a handler from this UI. */
+ /** Detaches a handler from this UI. */
def detachHandler(handler: ServletContextHandler) {
handlers -= handler
serverInfo.foreach(_.removeHandler(handler))
}
/**
- * Add a handler for static content.
+ * Adds a handler for static content.
*
* @param resourceBase Root of where to find resources to serve.
* @param path Path in UI where to mount the resources.
*/
- def addStaticHandler(resourceBase: String, path: String): Unit = {
+ def addStaticHandler(resourceBase: String, path: String = "/static"):
Unit = {
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 = {
--- End diff --
OK...since @vanzin requested I'm gonna make all the other changes while at
it :)
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21510: [SPARK-24490][WebUI] Use WebUI.addStaticHandler i...
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/21510#discussion_r194484175
--- Diff: core/src/main/scala/org/apache/spark/ui/WebUI.scala ---
@@ -88,41 +90,41 @@ private[spark] abstract class WebUI(
handlers += renderHandler
}
- /** Attach a handler to this UI. */
+ /** Attaches a handler to this UI. */
def attachHandler(handler: ServletContextHandler) {
handlers += handler
serverInfo.foreach(_.addHandler(handler))
}
- /** Detach a handler from this UI. */
+ /** Detaches a handler from this UI. */
def detachHandler(handler: ServletContextHandler) {
handlers -= handler
serverInfo.foreach(_.removeHandler(handler))
}
/**
- * Add a handler for static content.
+ * Adds a handler for static content.
*
* @param resourceBase Root of where to find resources to serve.
* @param path Path in UI where to mount the resources.
*/
- def addStaticHandler(resourceBase: String, path: String): Unit = {
+ def addStaticHandler(resourceBase: String, path: String = "/static"):
Unit = {
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 = {
--- End diff --
This method name is misleading. It will remove any handler, not just
"static" handlers... and since you're touching this code...
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21510: [SPARK-24490][WebUI] Use WebUI.addStaticHandler i...
Github user jerryshao commented on a diff in the pull request: https://github.com/apache/spark/pull/21510#discussion_r194309780 --- Diff: core/src/main/scala/org/apache/spark/ui/WebUI.scala --- @@ -101,12 +101,12 @@ private[spark] abstract class WebUI( } /** - * Add a handler for static content. + * Adds a handler for static content. --- End diff -- @jaceklaskowski please address this super tiny issue. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21510: [SPARK-24490][WebUI] Use WebUI.addStaticHandler i...
Github user kiszk commented on a diff in the pull request: https://github.com/apache/spark/pull/21510#discussion_r194266907 --- Diff: core/src/main/scala/org/apache/spark/ui/WebUI.scala --- @@ -101,12 +101,12 @@ private[spark] abstract class WebUI( } /** - * Add a handler for static content. + * Adds a handler for static content. --- End diff -- In this file, `s` is not added. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21510: [SPARK-24490][WebUI] Use WebUI.addStaticHandler i...
GitHub user jaceklaskowski opened a pull request: https://github.com/apache/spark/pull/21510 [SPARK-24490][WebUI] Use WebUI.addStaticHandler in web UIs `WebUI` defines `addStaticHandler` that web UIs don't use (and simply introduce duplication). Let's clean them up and remove duplications. Local build and waiting for Jenkins You can merge this pull request into a Git repository by running: $ git pull https://github.com/jaceklaskowski/spark SPARK-24490-Use-WebUI.addStaticHandler Alternatively you can review and apply these changes as the patch at: https://github.com/apache/spark/pull/21510.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #21510 commit 58a9ec42402cc92675e3e057309a803d08fd0cd7 Author: Jacek Laskowski Date: 2018-06-07T21:56:38Z [SPARK-24490][WebUI] Use WebUI.addStaticHandler in web UIs Closes https://issues.apache.org/jira/browse/SPARK-24490 --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
