tgravescs commented on a change in pull request #28528:
URL: https://github.com/apache/spark/pull/28528#discussion_r487946798
##########
File path: core/src/main/scala/org/apache/spark/SparkContext.scala
##########
@@ -623,6 +623,9 @@ class SparkContext(config: SparkConf) extends Logging {
// Post init
_taskScheduler.postStartHook()
+ if (isLocal) {
+ _env.metricsSystem.registerSource(Executor.executorSource)
Review comment:
So I haven't went and looked but if I don't configure the metrics system
files does this cause any overhead?
##########
File path: core/src/main/scala/org/apache/spark/executor/Executor.scala
##########
@@ -135,6 +135,8 @@ private[spark] class Executor(
env.metricsSystem.registerSource(new JVMCPUSource())
executorMetricsSource.foreach(_.register(env.metricsSystem))
env.metricsSystem.registerSource(env.blockManager.shuffleMetricsSource)
+ } else {
+ Executor.executorSource = executorSource
Review comment:
we now have 2 executorSource variables, one in the object and one in the
class. That seems a bit weird/confusing. I see the reason why but perhaps we
can rename the object one to have LocalModeOnly in the name or something as to
not accidentally be used in non-local mode.
##########
File path: core/src/main/scala/org/apache/spark/executor/Executor.scala
##########
@@ -134,8 +134,11 @@ private[spark] class Executor(
env.metricsSystem.registerSource(new JVMCPUSource())
executorMetricsSource.foreach(_.register(env.metricsSystem))
env.metricsSystem.registerSource(env.blockManager.shuffleMetricsSource)
+ } else {
+ Executor.executorSource = executorSource
Review comment:
I can think of other ways to do this more cleanly but it would be quite
a bit more code change. I do think we should explicitly make a comment about
the appId so that someone who comes and looks at this later realizes that
##########
File path: docs/monitoring.md
##########
@@ -1153,6 +1153,11 @@ This is the component with the largest amount of
instrumented metrics
- namespace=JVMCPU
- jvmCpuTime
+- namespace=executor
Review comment:
this looks the same as the ExecutorMetrics but I think they are actually
different in that this doesn't give you the JVM metrics - correct? Perhaps we
need to update the one below.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
