[GitHub] [spark] venkata91 commented on a change in pull request #34695: [SPARK-32446][CORE] Add percentile distribution REST API & UI of peak memory metrics for all executors

[GitBox <gi...@apache.org>]

venkata91 commented on a change in pull request #34695:
URL: https://github.com/apache/spark/pull/34695#discussion_r770073821



##########
File path: core/src/main/scala/org/apache/spark/status/AppStatusStore.scala
##########
@@ -402,6 +402,18 @@ private[spark] class AppStatusStore(
     Some(computedQuantiles)
   }
 
+  /**
+   * Calculates a summary of the executor metrics for executors, returning the
+   * requested quantiles for the recorded metrics.
+   */
+  def executorMetricSummary(
+    activeOnly: Boolean,

Review comment:
       nit: indent off.

##########
File path: core/src/main/resources/org/apache/spark/ui/static/executorspage.js
##########
@@ -770,5 +940,140 @@ $(document).ready(function () {
         }
       });
     });
+
+    var quantiles = "0,0.25,0.5,0.75,1.0";
+    $.getJSON(createRESTEndPointForExecutorsPeakMetricsSummariesPage(appId) + "?activeOnly=true&quantiles=" + quantiles,
+      function(executorMetricsResponse, _ignored_status, _ignored_jqXHR) {
+        var taskMetricKeys = Object.keys(executorMetricsResponse);
+        taskMetricKeys.forEach(function (columnKey) {
+          var row;
+          switch(columnKey) {
+
+            case "JVMHeapMemory":

Review comment:
       Why do we need so many `case` statements here, seems like except for the `default` case all the case statements pretty much doing the same thing?
   If you only want to do this for select `columnKey`s, can we just add them to a `Set` and check if it is there and make the call to get the `row` accordingly?

##########
File path: core/src/main/resources/org/apache/spark/ui/static/utils.js
##########
@@ -205,6 +205,29 @@ function createRESTEndPointForExecutorsPage(appId) {
   return uiRoot + "/api/v1/applications/" + appId + "/allexecutors";
 }
 
+function createRESTEndPointForExecutorsPeakMetricsSummariesPage(appId) {
+  var words = getBaseURI().split('/');

Review comment:
       It seems like most of the logic here is duplicated with `createRESTEndPointForExecutorsPage` except for the REST call. Can we refactor it a bit and avoid/reduce the redundant code?

##########
File path: core/src/main/resources/org/apache/spark/ui/static/executorspage.js
##########
@@ -770,5 +940,140 @@ $(document).ready(function () {
         }
       });
     });
+
+    var quantiles = "0,0.25,0.5,0.75,1.0";
+    $.getJSON(createRESTEndPointForExecutorsPeakMetricsSummariesPage(appId) + "?activeOnly=true&quantiles=" + quantiles,

Review comment:
       nit: can we have a smaller method name than this one? for eg: `createRESTEndPointForExecutorsSummaries` or some other smaller name?

##########
File path: core/src/main/scala/org/apache/spark/status/api/v1/OneApplicationResource.scala
##########
@@ -52,6 +52,25 @@ private[v1] class AbstractApplicationResource extends BaseAppResource {
   @Path("executors")
   def executorList(): Seq[ExecutorSummary] = withUI(_.store.executorList(true))
 
+  @GET
+  @Path("executorPeakMemoryMetricsDistribution")
+  def executorSummary(
+    @QueryParam("activeOnly") @DefaultValue("true") activeOnly: Boolean,
+    @DefaultValue("0.05,0.25,0.5,0.75,0.95") @QueryParam("quantiles") quantileString: String)
+  : ExecutorPeakMetricsDistributions = withUI { ui =>
+    val quantiles = quantileString.split(",").map { s =>
+      try {
+        s.toDouble
+      } catch {
+        case nfe: NumberFormatException =>

Review comment:
       nit: seems like `nfe` is not used here, use `_` placeholder?

##########
File path: core/src/main/scala/org/apache/spark/status/api/v1/OneApplicationResource.scala
##########
@@ -52,6 +52,25 @@ private[v1] class AbstractApplicationResource extends BaseAppResource {
   @Path("executors")
   def executorList(): Seq[ExecutorSummary] = withUI(_.store.executorList(true))
 
+  @GET
+  @Path("executorPeakMemoryMetricsDistribution")
+  def executorSummary(
+    @QueryParam("activeOnly") @DefaultValue("true") activeOnly: Boolean,

Review comment:
       nit: indent off




-- 
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.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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