Github user squito commented on a diff in the pull request:
https://github.com/apache/spark/pull/22612#discussion_r222402317
--- Diff:
core/src/main/scala/org/apache/spark/metrics/ExecutorMetricType.scala ---
@@ -59,6 +60,43 @@ case object JVMOffHeapMemory extends ExecutorMetricType {
}
}
+case object ProcessTreeJVMRSSMemory extends ExecutorMetricType {
+ override private[spark] def getMetricValue(memoryManager:
MemoryManager): Long = {
+ ExecutorMetricType.pTreeInfo.updateAllMetrics()
--- End diff --
I still don't like how this is actually updating all the other metrics --
it makes the code more confusing to follow, as you have to know there is a
relationship between all of the metrics. I understand that you want to do the
work once and grab all the metrics, but we should find a better way to do that.
I see how the current api makes that hard to do.
I have two suggestions:
1) change the api to have `getMetricValue()` also pass in the
System.currentTimeInMillis(). Then every single metric type would compare the
passed in time against the last time you computed the metrics -- if it was
stale, it would recompute everything and update the time.
2) Change the api to allow one "metric getter" object to actually supply
multiple metrics. You'd have a simple implementation which woudl just provide
one metric, and you'd change all the existing metrics to extend that simple
case, but your implementation would provide multiple metrics in one go.
I actually think (2) is better (that is what I did in memory-monitor
plugin) though its a bit more work. You might need to play with this a bit.
thoughts @edwinalu @mccheah ?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
