[GitHub] spark pull request #22034: [SPARK-25054][CORE] Enable MetricsServlet sink fo...

2018-08-09 Thread LantaoJin
Github user LantaoJin closed the pull request at:

https://github.com/apache/spark/pull/22034


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #22034: [SPARK-25054][CORE] Enable MetricsServlet sink fo...

2018-08-08 Thread jerryshao
Github user jerryshao commented on a diff in the pull request:

https://github.com/apache/spark/pull/22034#discussion_r208786793
  
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -169,6 +171,19 @@ private[spark] class Executor(
 
   startDriverHeartbeater()
 
+  /**
+   * We add an empty WebUI in executor to enable Executor MetricsServlet 
sink if needed.
+   */
+  private val executorWebUiEnabled = 
conf.getBoolean("spark.executor.ui.enabled", false)
+  private[executor] var webUi: ExecutorWebUI = _
+  if (executorWebUiEnabled && !isLocal) {
+ webUi = new ExecutorWebUI(conf, env.securityManager, 
SparkUI.getUIPort(conf))
+ webUi.bind()
+ env.metricsSystem.getServletHandlers.foreach(webUi.attachHandler)
+ heartbeatReceiverRef.ask[Boolean](ReportExecutorWebUrl(executorId, 
webUi.webUrl))
+ logInfo(s"Starting executor web ui at ${webUi.webUrl}")
--- End diff --

Maybe you can use jmx or some other tools. Basically the metrics is still 
report in every N seconds, so pulling frequently doesn't help increasing the 
accuracy.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #22034: [SPARK-25054][CORE] Enable MetricsServlet sink fo...

2018-08-08 Thread LantaoJin
Github user LantaoJin commented on a diff in the pull request:

https://github.com/apache/spark/pull/22034#discussion_r208784817
  
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -169,6 +171,19 @@ private[spark] class Executor(
 
   startDriverHeartbeater()
 
+  /**
+   * We add an empty WebUI in executor to enable Executor MetricsServlet 
sink if needed.
+   */
+  private val executorWebUiEnabled = 
conf.getBoolean("spark.executor.ui.enabled", false)
+  private[executor] var webUi: ExecutorWebUI = _
+  if (executorWebUiEnabled && !isLocal) {
+ webUi = new ExecutorWebUI(conf, env.securityManager, 
SparkUI.getUIPort(conf))
+ webUi.bind()
+ env.metricsSystem.getServletHandlers.foreach(webUi.attachHandler)
+ heartbeatReceiverRef.ask[Boolean](ReportExecutorWebUrl(executorId, 
webUi.webUrl))
+ logInfo(s"Starting executor web ui at ${webUi.webUrl}")
--- End diff --

Thanks @jerryshao . It's overkill for this purpose. But it offers a pull 
way to get executor metrics. I will think twice how to keep a pull way to get 
that.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #22034: [SPARK-25054][CORE] Enable MetricsServlet sink fo...

2018-08-08 Thread jerryshao
Github user jerryshao commented on a diff in the pull request:

https://github.com/apache/spark/pull/22034#discussion_r208552871
  
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -169,6 +171,19 @@ private[spark] class Executor(
 
   startDriverHeartbeater()
 
+  /**
+   * We add an empty WebUI in executor to enable Executor MetricsServlet 
sink if needed.
+   */
+  private val executorWebUiEnabled = 
conf.getBoolean("spark.executor.ui.enabled", false)
+  private[executor] var webUi: ExecutorWebUI = _
+  if (executorWebUiEnabled && !isLocal) {
+ webUi = new ExecutorWebUI(conf, env.securityManager, 
SparkUI.getUIPort(conf))
+ webUi.bind()
+ env.metricsSystem.getServletHandlers.foreach(webUi.attachHandler)
+ heartbeatReceiverRef.ask[Boolean](ReportExecutorWebUrl(executorId, 
webUi.webUrl))
+ logInfo(s"Starting executor web ui at ${webUi.webUrl}")
--- End diff --

It is too overkill to start an jetty server on each executor only for 
metrics. I believe you have many different ways to collect executor metrics 
other than servlet. 

Besides, to get executor URL you add a new heartbeat message, basically I 
think it is too overkill.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark pull request #22034: [SPARK-25054][CORE] Enable MetricsServlet sink fo...

2018-08-07 Thread LantaoJin
GitHub user LantaoJin opened a pull request:

https://github.com/apache/spark/pull/22034

[SPARK-25054][CORE] Enable MetricsServlet sink for Executor

## What changes were proposed in this pull request?

The MetricsServlet sink is added by default as a sink in the master. But 
there is no way to query the Executor metrics via Servlet. This ticket offers a 
way to enable the MetricsServlet Sink in Executor side when 
spark.executor.ui.enabled is set to true.

## How was this patch tested?

Unit tests


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/LantaoJin/spark SPARK-25054

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/spark/pull/22034.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 #22034


commit f446fccfcd803f5081e1f994401ce86a474b9fff
Author: LantaoJin 
Date:   2018-08-08T04:07:36Z

[SPARK-25054][CORE] Enable MetricsServlet sink for Executor




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org