You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by mareksimunek <gi...@git.apache.org> on 2018/09/20 08:04:16 UTC
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
GitHub user mareksimunek opened a pull request:
https://github.com/apache/spark/pull/22485
[SPARK-18364][YARN] Expose metrics for YarnShuffleService
## What changes were proposed in this pull request?
This PR is follow-up of closed https://github.com/apache/spark/pull/17401 which only ended due to of inactivity, but its still nice feature to have.
Given review by @jerryshao taken in consideration and edited:
- @VisibleForTesting deleted because of dependency conflicts
- removed unnecessary reflection for `MetricsSystemImpl`
- added more available types for gauge
## How was this patch tested?
Manual deploy of new yarn-shuffle jar into a Node Manager and verifying that the metrics appear in the Node Manager-standard location. This is JMX with an query endpoint running on `hostname:port`
Resulting metrics look like this:
```
curl -sk -XGET hostname:port | grep -v '#' | grep 'shuffleService'
hadoop_nodemanager_openblockrequestlatencymillis_rate15{name="shuffleService",} 0.31428910657834713
hadoop_nodemanager_blocktransferratebytes_rate15{name="shuffleService",} 566144.9983653595
hadoop_nodemanager_blocktransferratebytes_ratemean{name="shuffleService",} 2464409.9678099006
hadoop_nodemanager_openblockrequestlatencymillis_rate1{name="shuffleService",} 1.2893844732240272
hadoop_nodemanager_registeredexecutorssize{name="shuffleService",} 2.0
hadoop_nodemanager_openblockrequestlatencymillis_ratemean{name="shuffleService",} 1.255574678369966
hadoop_nodemanager_openblockrequestlatencymillis_count{name="shuffleService",} 315.0
hadoop_nodemanager_openblockrequestlatencymillis_rate5{name="shuffleService",} 0.7661929192569739
hadoop_nodemanager_registerexecutorrequestlatencymillis_ratemean{name="shuffleService",} 0.0
hadoop_nodemanager_registerexecutorrequestlatencymillis_count{name="shuffleService",} 0.0
hadoop_nodemanager_registerexecutorrequestlatencymillis_rate1{name="shuffleService",} 0.0
hadoop_nodemanager_registerexecutorrequestlatencymillis_rate5{name="shuffleService",} 0.0
hadoop_nodemanager_blocktransferratebytes_count{name="shuffleService",} 6.18271213E8
hadoop_nodemanager_registerexecutorrequestlatencymillis_rate15{name="shuffleService",} 0.0
hadoop_nodemanager_blocktransferratebytes_rate5{name="shuffleService",} 1154114.4881816586
hadoop_nodemanager_blocktransferratebytes_rate1{name="shuffleService",} 574745.0749848988
```
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/mareksimunek/spark SPARK-18364
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/22485.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 #22485
----
commit 5d9434bbb2fa650e987bc2e68d183aea691f9ac5
Author: Andrew Ash <an...@...>
Date: 2017-03-23T02:59:38Z
[SPARK-18364][YARN] Expose metrics for YarnShuffleService
Registers the shuffle server's metrics with the Hadoop Node Manager's
DefaultMetricsSystem.
Test metric collector gets right converted calls
camel-case shuffleService
Pass scalastyle
Reformat and organize imports
With import order specified at http://spark.apache.org/contributing.html
commit 6c96397536af57a8bbe8dd2529547427f643512b
Author: marek.simunek <ma...@...>
Date: 2018-09-19T15:17:53Z
[SPARK-18364][YARN] YarnShuffleService metrics correction
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/22485#discussion_r219508537
--- Diff: resource-managers/yarn/src/test/scala/org/apache/spark/network/yarn/YarnShuffleServiceMetricsSuite.scala ---
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.network.yarn
+
+import scala.collection.JavaConverters._
+
+import org.apache.hadoop.metrics2.MetricsRecordBuilder
+import org.mockito.Matchers._
+import org.mockito.Mockito.{mock, times, verify, when}
+import org.scalatest.Matchers
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.network.server.OneForOneStreamManager
+import org.apache.spark.network.shuffle.{ExternalShuffleBlockHandler, ExternalShuffleBlockResolver}
+
+class YarnShuffleServiceMetricsSuite extends SparkFunSuite with Matchers {
+
+ val streamManager = mock(classOf[OneForOneStreamManager])
+ val blockResolver = mock(classOf[ExternalShuffleBlockResolver])
+ when(blockResolver.getRegisteredExecutorsSize).thenReturn(42)
+
+ val metrics = new ExternalShuffleBlockHandler(streamManager, blockResolver).getAllMetrics
+
+ test("metrics named as expected") {
+ val allMetrics = Set(
+ "openBlockRequestLatencyMillis", "registerExecutorRequestLatencyMillis",
+ "blockTransferRateBytes", "registeredExecutorsSize")
+
+ metrics.getMetrics.keySet().asScala should be (allMetrics)
+ }
+
+ // these three metrics have the same effect on the collector
+ for (testname <- Seq("openBlockRequestLatencyMillis",
+ "registerExecutorRequestLatencyMillis",
+ "blockTransferRateBytes")) {
+ test(s"$testname - collector receives correct types") {
+ val builder = mock(classOf[MetricsRecordBuilder])
+ when(builder.addCounter(any(), anyLong())).thenReturn(builder)
+ when(builder.addGauge(any(), anyDouble())).thenReturn(builder)
+
+ YarnShuffleServiceMetrics.collectMetric(builder, testname,
+ metrics.getMetrics.get(testname))
+
+ verify(builder).addCounter(anyObject(), anyLong())
+ verify(builder, times(4)).addGauge(anyObject(), anyDouble())
+ }
+ }
+
+ // this metric writes only one gauge to the collector
+ test("registeredExecutorsSize - collector receives correct types") {
+ val builder = mock(classOf[MetricsRecordBuilder])
+ when(builder.addCounter(any(), anyLong())).thenReturn(builder)
+ when(builder.addGauge(any(), anyDouble())).thenReturn(builder)
+
+ YarnShuffleServiceMetrics.collectMetric(builder, "registeredExecutorsSize",
--- End diff --
you are calling a private function here
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22485
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96357/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by mareksimunek <gi...@git.apache.org>.
Github user mareksimunek commented on the issue:
https://github.com/apache/spark/pull/22485
ok to test
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22485
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the issue:
https://github.com/apache/spark/pull/22485
merged to master, thanks @mareksimunek
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22485
**[Test build #96508 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96508/testReport)** for PR 22485 at commit [`764404a`](https://github.com/apache/spark/commit/764404a89eacc58898a3fff2e49b00ac2c1a01b3).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/22485#discussion_r220185883
--- Diff: common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleService.java ---
@@ -168,6 +170,15 @@ protected void serviceInit(Configuration conf) throws Exception {
TransportConf transportConf = new TransportConf("shuffle", new HadoopConfigProvider(conf));
blockHandler = new ExternalShuffleBlockHandler(transportConf, registeredExecutorFile);
+ // register metrics on the block handler into the Node Manager's metrics system.
+ YarnShuffleServiceMetrics serviceMetrics =
+ new YarnShuffleServiceMetrics(blockHandler.getAllMetrics());
+
+ MetricsSystemImpl metricsSystem = (MetricsSystemImpl) DefaultMetricsSystem.instance();
+ metricsSystem.register(
+ "shuffleService", "Metrics on the Spark Shuffle Service", serviceMetrics);
--- End diff --
Probably best to make this sparkshuffleservice as well.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the issue:
https://github.com/apache/spark/pull/22485
ok to test
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by ash211 <gi...@git.apache.org>.
Github user ash211 commented on a diff in the pull request:
https://github.com/apache/spark/pull/22485#discussion_r219916796
--- Diff: common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleService.java ---
@@ -168,6 +170,15 @@ protected void serviceInit(Configuration conf) throws Exception {
TransportConf transportConf = new TransportConf("shuffle", new HadoopConfigProvider(conf));
blockHandler = new ExternalShuffleBlockHandler(transportConf, registeredExecutorFile);
+ // register metrics on the block handler into the Node Manager's metrics system.
+ YarnShuffleServiceMetrics serviceMetrics =
+ new YarnShuffleServiceMetrics(blockHandler.getAllMetrics());
+
+ MetricsSystemImpl metricsSystem = (MetricsSystemImpl) DefaultMetricsSystem.instance();
+ metricsSystem.register(
--- End diff --
Sorry for the delay.
My explanation for using reflection was here: https://github.com/palantir/spark/pull/149/files#r107770857 -- basically `registerSource` isn't accessible from here.
As to why use `registerSource` vs `register`, I don't think I knew about `register` at the time. Looking at https://github.com/apache/hadoop/blame/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/metrics2/impl/MetricsSystemImpl.java `registerSource` ends up being called both ways.
If you can get the right behavior using the accessible method rather than doing reflection, I'd say we should go with that way.
cc @robert3005 @mccheah fysa for potential future merge conflict
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by mareksimunek <gi...@git.apache.org>.
Github user mareksimunek commented on a diff in the pull request:
https://github.com/apache/spark/pull/22485#discussion_r220137523
--- Diff: common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleServiceMetrics.java ---
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.network.yarn;
+
+import com.codahale.metrics.*;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.spark.network.shuffle.ExternalShuffleBlockHandler;
+
+import java.util.Map;
+
+/**
+ * Forward {@link ExternalShuffleBlockHandler.ShuffleMetrics} to hadoop metrics system.
+ * NodeManager by default exposes JMX endpoint where can be collected.
+ */
+class YarnShuffleServiceMetrics implements MetricsSource {
+
+ private final MetricSet metricSet;
+
+ YarnShuffleServiceMetrics(MetricSet metricSet) {
+ this.metricSet = metricSet;
+ }
+
+ /**
+ * Get metrics from the source
+ *
+ * @param collector to contain the resulting metrics snapshot
+ * @param all if true, return all metrics even if unchanged.
+ */
+ @Override
+ public void getMetrics(MetricsCollector collector, boolean all) {
+ MetricsRecordBuilder metricsRecordBuilder = collector.addRecord("shuffleService");
+
+ for (Map.Entry<String, Metric> entry : metricSet.getMetrics().entrySet()) {
+ collectMetric(metricsRecordBuilder, entry.getKey(), entry.getValue());
+ }
+ }
+
+ /**
+ * The metric types used in {@link ExternalShuffleBlockHandler.ShuffleMetrics}
+ */
--- End diff --
added
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by mareksimunek <gi...@git.apache.org>.
Github user mareksimunek commented on a diff in the pull request:
https://github.com/apache/spark/pull/22485#discussion_r219786966
--- Diff: resource-managers/yarn/src/test/scala/org/apache/spark/network/yarn/YarnShuffleServiceMetricsSuite.scala ---
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.network.yarn
+
+import scala.collection.JavaConverters._
+
+import org.apache.hadoop.metrics2.MetricsRecordBuilder
+import org.mockito.Matchers._
+import org.mockito.Mockito.{mock, times, verify, when}
+import org.scalatest.Matchers
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.network.server.OneForOneStreamManager
+import org.apache.spark.network.shuffle.{ExternalShuffleBlockHandler, ExternalShuffleBlockResolver}
+
+class YarnShuffleServiceMetricsSuite extends SparkFunSuite with Matchers {
+
+ val streamManager = mock(classOf[OneForOneStreamManager])
+ val blockResolver = mock(classOf[ExternalShuffleBlockResolver])
+ when(blockResolver.getRegisteredExecutorsSize).thenReturn(42)
+
+ val metrics = new ExternalShuffleBlockHandler(streamManager, blockResolver).getAllMetrics
+
+ test("metrics named as expected") {
+ val allMetrics = Set(
+ "openBlockRequestLatencyMillis", "registerExecutorRequestLatencyMillis",
+ "blockTransferRateBytes", "registeredExecutorsSize")
+
+ metrics.getMetrics.keySet().asScala should be (allMetrics)
+ }
+
+ // these three metrics have the same effect on the collector
+ for (testname <- Seq("openBlockRequestLatencyMillis",
+ "registerExecutorRequestLatencyMillis",
+ "blockTransferRateBytes")) {
+ test(s"$testname - collector receives correct types") {
+ val builder = mock(classOf[MetricsRecordBuilder])
+ when(builder.addCounter(any(), anyLong())).thenReturn(builder)
+ when(builder.addGauge(any(), anyDouble())).thenReturn(builder)
+
+ YarnShuffleServiceMetrics.collectMetric(builder, testname,
--- End diff --
fixed
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the issue:
https://github.com/apache/spark/pull/22485
test this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22485
**[Test build #96508 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96508/testReport)** for PR 22485 at commit [`764404a`](https://github.com/apache/spark/commit/764404a89eacc58898a3fff2e49b00ac2c1a01b3).
* This patch **fails Java style tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22485
**[Test build #96357 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96357/testReport)** for PR 22485 at commit [`6c96397`](https://github.com/apache/spark/commit/6c96397536af57a8bbe8dd2529547427f643512b).
* This patch **fails to build**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `class YarnShuffleServiceMetrics implements MetricsSource `
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by mareksimunek <gi...@git.apache.org>.
Github user mareksimunek commented on a diff in the pull request:
https://github.com/apache/spark/pull/22485#discussion_r219786943
--- Diff: resource-managers/yarn/src/test/scala/org/apache/spark/network/yarn/YarnShuffleServiceMetricsSuite.scala ---
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.network.yarn
+
+import scala.collection.JavaConverters._
+
+import org.apache.hadoop.metrics2.MetricsRecordBuilder
+import org.mockito.Matchers._
+import org.mockito.Mockito.{mock, times, verify, when}
+import org.scalatest.Matchers
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.network.server.OneForOneStreamManager
+import org.apache.spark.network.shuffle.{ExternalShuffleBlockHandler, ExternalShuffleBlockResolver}
+
+class YarnShuffleServiceMetricsSuite extends SparkFunSuite with Matchers {
+
+ val streamManager = mock(classOf[OneForOneStreamManager])
+ val blockResolver = mock(classOf[ExternalShuffleBlockResolver])
+ when(blockResolver.getRegisteredExecutorsSize).thenReturn(42)
+
+ val metrics = new ExternalShuffleBlockHandler(streamManager, blockResolver).getAllMetrics
+
+ test("metrics named as expected") {
+ val allMetrics = Set(
+ "openBlockRequestLatencyMillis", "registerExecutorRequestLatencyMillis",
+ "blockTransferRateBytes", "registeredExecutorsSize")
+
+ metrics.getMetrics.keySet().asScala should be (allMetrics)
+ }
+
+ // these three metrics have the same effect on the collector
+ for (testname <- Seq("openBlockRequestLatencyMillis",
+ "registerExecutorRequestLatencyMillis",
+ "blockTransferRateBytes")) {
+ test(s"$testname - collector receives correct types") {
+ val builder = mock(classOf[MetricsRecordBuilder])
+ when(builder.addCounter(any(), anyLong())).thenReturn(builder)
+ when(builder.addGauge(any(), anyDouble())).thenReturn(builder)
+
+ YarnShuffleServiceMetrics.collectMetric(builder, testname,
+ metrics.getMetrics.get(testname))
+
+ verify(builder).addCounter(anyObject(), anyLong())
+ verify(builder, times(4)).addGauge(anyObject(), anyDouble())
+ }
+ }
+
+ // this metric writes only one gauge to the collector
+ test("registeredExecutorsSize - collector receives correct types") {
+ val builder = mock(classOf[MetricsRecordBuilder])
+ when(builder.addCounter(any(), anyLong())).thenReturn(builder)
--- End diff --
deleted this wasnt used.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by mareksimunek <gi...@git.apache.org>.
Github user mareksimunek commented on a diff in the pull request:
https://github.com/apache/spark/pull/22485#discussion_r220137410
--- Diff: common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleServiceMetrics.java ---
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.network.yarn;
+
+import com.codahale.metrics.*;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
--- End diff --
spark import removed because of checkstyle wont allow to have import in javadoc when you have `{@link className`} syntax. I think checkstyle rules should be corrected.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22485
**[Test build #96516 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96516/testReport)** for PR 22485 at commit [`764404a`](https://github.com/apache/spark/commit/764404a89eacc58898a3fff2e49b00ac2c1a01b3).
* This patch **fails Java style tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by mareksimunek <gi...@git.apache.org>.
Github user mareksimunek commented on a diff in the pull request:
https://github.com/apache/spark/pull/22485#discussion_r219786477
--- Diff: common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleService.java ---
@@ -168,6 +170,15 @@ protected void serviceInit(Configuration conf) throws Exception {
TransportConf transportConf = new TransportConf("shuffle", new HadoopConfigProvider(conf));
blockHandler = new ExternalShuffleBlockHandler(transportConf, registeredExecutorFile);
+ // register metrics on the block handler into the Node Manager's metrics system.
+ YarnShuffleServiceMetrics serviceMetrics =
+ new YarnShuffleServiceMetrics(blockHandler.getAllMetrics());
--- End diff --
fixed
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by mareksimunek <gi...@git.apache.org>.
Github user mareksimunek commented on a diff in the pull request:
https://github.com/apache/spark/pull/22485#discussion_r219789235
--- Diff: common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleService.java ---
@@ -168,6 +170,15 @@ protected void serviceInit(Configuration conf) throws Exception {
TransportConf transportConf = new TransportConf("shuffle", new HadoopConfigProvider(conf));
blockHandler = new ExternalShuffleBlockHandler(transportConf, registeredExecutorFile);
+ // register metrics on the block handler into the Node Manager's metrics system.
+ YarnShuffleServiceMetrics serviceMetrics =
+ new YarnShuffleServiceMetrics(blockHandler.getAllMetrics());
+
+ MetricsSystemImpl metricsSystem = (MetricsSystemImpl) DefaultMetricsSystem.instance();
+ metricsSystem.register(
--- End diff --
`register` method only did some fancy stuff on metrics name, so I didnt understand the motivation to use `registerSource`. Also I think Ash only follows notifications on JIRA ticket so I notified him there.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by mareksimunek <gi...@git.apache.org>.
Github user mareksimunek commented on the issue:
https://github.com/apache/spark/pull/22485
test this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by mareksimunek <gi...@git.apache.org>.
Github user mareksimunek commented on the issue:
https://github.com/apache/spark/pull/22485
@tgravescs thanks for review, I made corrections for your notes and probably need some permissions to retest. Could you please test this PR again?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22485
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96547/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the issue:
https://github.com/apache/spark/pull/22485
ok to test
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22485
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22485
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by mareksimunek <gi...@git.apache.org>.
Github user mareksimunek commented on a diff in the pull request:
https://github.com/apache/spark/pull/22485#discussion_r219786991
--- Diff: resource-managers/yarn/src/test/scala/org/apache/spark/network/yarn/YarnShuffleServiceMetricsSuite.scala ---
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.network.yarn
+
+import scala.collection.JavaConverters._
+
+import org.apache.hadoop.metrics2.MetricsRecordBuilder
+import org.mockito.Matchers._
+import org.mockito.Mockito.{mock, times, verify, when}
+import org.scalatest.Matchers
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.network.server.OneForOneStreamManager
+import org.apache.spark.network.shuffle.{ExternalShuffleBlockHandler, ExternalShuffleBlockResolver}
+
+class YarnShuffleServiceMetricsSuite extends SparkFunSuite with Matchers {
+
+ val streamManager = mock(classOf[OneForOneStreamManager])
+ val blockResolver = mock(classOf[ExternalShuffleBlockResolver])
+ when(blockResolver.getRegisteredExecutorsSize).thenReturn(42)
+
+ val metrics = new ExternalShuffleBlockHandler(streamManager, blockResolver).getAllMetrics
+
+ test("metrics named as expected") {
+ val allMetrics = Set(
+ "openBlockRequestLatencyMillis", "registerExecutorRequestLatencyMillis",
+ "blockTransferRateBytes", "registeredExecutorsSize")
+
+ metrics.getMetrics.keySet().asScala should be (allMetrics)
+ }
+
+ // these three metrics have the same effect on the collector
+ for (testname <- Seq("openBlockRequestLatencyMillis",
+ "registerExecutorRequestLatencyMillis",
+ "blockTransferRateBytes")) {
+ test(s"$testname - collector receives correct types") {
+ val builder = mock(classOf[MetricsRecordBuilder])
+ when(builder.addCounter(any(), anyLong())).thenReturn(builder)
+ when(builder.addGauge(any(), anyDouble())).thenReturn(builder)
+
+ YarnShuffleServiceMetrics.collectMetric(builder, testname,
+ metrics.getMetrics.get(testname))
+
+ verify(builder).addCounter(anyObject(), anyLong())
+ verify(builder, times(4)).addGauge(anyObject(), anyDouble())
+ }
+ }
+
+ // this metric writes only one gauge to the collector
+ test("registeredExecutorsSize - collector receives correct types") {
+ val builder = mock(classOf[MetricsRecordBuilder])
+ when(builder.addCounter(any(), anyLong())).thenReturn(builder)
+ when(builder.addGauge(any(), anyDouble())).thenReturn(builder)
+
+ YarnShuffleServiceMetrics.collectMetric(builder, "registeredExecutorsSize",
--- End diff --
fixed
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22485
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by mareksimunek <gi...@git.apache.org>.
Github user mareksimunek commented on a diff in the pull request:
https://github.com/apache/spark/pull/22485#discussion_r219786856
--- Diff: resource-managers/yarn/src/test/scala/org/apache/spark/network/yarn/YarnShuffleServiceMetricsSuite.scala ---
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.network.yarn
+
+import scala.collection.JavaConverters._
+
+import org.apache.hadoop.metrics2.MetricsRecordBuilder
+import org.mockito.Matchers._
+import org.mockito.Mockito.{mock, times, verify, when}
+import org.scalatest.Matchers
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.network.server.OneForOneStreamManager
+import org.apache.spark.network.shuffle.{ExternalShuffleBlockHandler, ExternalShuffleBlockResolver}
+
+class YarnShuffleServiceMetricsSuite extends SparkFunSuite with Matchers {
+
+ val streamManager = mock(classOf[OneForOneStreamManager])
+ val blockResolver = mock(classOf[ExternalShuffleBlockResolver])
+ when(blockResolver.getRegisteredExecutorsSize).thenReturn(42)
+
+ val metrics = new ExternalShuffleBlockHandler(streamManager, blockResolver).getAllMetrics
+
+ test("metrics named as expected") {
+ val allMetrics = Set(
+ "openBlockRequestLatencyMillis", "registerExecutorRequestLatencyMillis",
+ "blockTransferRateBytes", "registeredExecutorsSize")
+
+ metrics.getMetrics.keySet().asScala should be (allMetrics)
+ }
+
+ // these three metrics have the same effect on the collector
+ for (testname <- Seq("openBlockRequestLatencyMillis",
+ "registerExecutorRequestLatencyMillis",
+ "blockTransferRateBytes")) {
+ test(s"$testname - collector receives correct types") {
+ val builder = mock(classOf[MetricsRecordBuilder])
+ when(builder.addCounter(any(), anyLong())).thenReturn(builder)
--- End diff --
these metrics are Timer or Meter so its called in `YarnShuffleServiceMetrics.collectMetrics`.
https://github.com/apache/spark/pull/22485/files#diff-04b0327ae6bce4204c03442b0e6d8718R63
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/22485#discussion_r219506086
--- Diff: resource-managers/yarn/src/test/scala/org/apache/spark/network/yarn/YarnShuffleServiceMetricsSuite.scala ---
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.network.yarn
+
+import scala.collection.JavaConverters._
+
+import org.apache.hadoop.metrics2.MetricsRecordBuilder
+import org.mockito.Matchers._
+import org.mockito.Mockito.{mock, times, verify, when}
+import org.scalatest.Matchers
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.network.server.OneForOneStreamManager
+import org.apache.spark.network.shuffle.{ExternalShuffleBlockHandler, ExternalShuffleBlockResolver}
+
+class YarnShuffleServiceMetricsSuite extends SparkFunSuite with Matchers {
+
+ val streamManager = mock(classOf[OneForOneStreamManager])
+ val blockResolver = mock(classOf[ExternalShuffleBlockResolver])
+ when(blockResolver.getRegisteredExecutorsSize).thenReturn(42)
+
+ val metrics = new ExternalShuffleBlockHandler(streamManager, blockResolver).getAllMetrics
+
+ test("metrics named as expected") {
+ val allMetrics = Set(
+ "openBlockRequestLatencyMillis", "registerExecutorRequestLatencyMillis",
+ "blockTransferRateBytes", "registeredExecutorsSize")
+
+ metrics.getMetrics.keySet().asScala should be (allMetrics)
+ }
+
+ // these three metrics have the same effect on the collector
+ for (testname <- Seq("openBlockRequestLatencyMillis",
+ "registerExecutorRequestLatencyMillis",
+ "blockTransferRateBytes")) {
+ test(s"$testname - collector receives correct types") {
+ val builder = mock(classOf[MetricsRecordBuilder])
+ when(builder.addCounter(any(), anyLong())).thenReturn(builder)
+ when(builder.addGauge(any(), anyDouble())).thenReturn(builder)
+
+ YarnShuffleServiceMetrics.collectMetric(builder, testname,
+ metrics.getMetrics.get(testname))
+
+ verify(builder).addCounter(anyObject(), anyLong())
+ verify(builder, times(4)).addGauge(anyObject(), anyDouble())
+ }
+ }
+
+ // this metric writes only one gauge to the collector
+ test("registeredExecutorsSize - collector receives correct types") {
+ val builder = mock(classOf[MetricsRecordBuilder])
+ when(builder.addCounter(any(), anyLong())).thenReturn(builder)
--- End diff --
Don't seem to use these?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22485
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96552/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22485
**[Test build #96552 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96552/testReport)** for PR 22485 at commit [`e259b6f`](https://github.com/apache/spark/commit/e259b6f899ab44e38309b34c58134772c2e591bf).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22485
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96508/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/22485#discussion_r219859567
--- Diff: common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleServiceMetrics.java ---
@@ -56,7 +56,7 @@ public void getMetrics(MetricsCollector collector, boolean all) {
/**
* The metric types used in {@link ExternalShuffleBlockHandler.ShuffleMetrics}
*/
- private static void collectMetric(MetricsRecordBuilder metricsRecordBuilder, String name, Metric metric) {
--- End diff --
we should add a comment to the java "Exposed for testing."
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by mareksimunek <gi...@git.apache.org>.
Github user mareksimunek commented on a diff in the pull request:
https://github.com/apache/spark/pull/22485#discussion_r220137502
--- Diff: common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleServiceMetrics.java ---
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.network.yarn;
+
+import com.codahale.metrics.*;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.spark.network.shuffle.ExternalShuffleBlockHandler;
+
+import java.util.Map;
+
+/**
+ * Forward {@link ExternalShuffleBlockHandler.ShuffleMetrics} to hadoop metrics system.
+ * NodeManager by default exposes JMX endpoint where can be collected.
+ */
+class YarnShuffleServiceMetrics implements MetricsSource {
+
+ private final MetricSet metricSet;
+
+ YarnShuffleServiceMetrics(MetricSet metricSet) {
+ this.metricSet = metricSet;
+ }
+
+ /**
+ * Get metrics from the source
+ *
+ * @param collector to contain the resulting metrics snapshot
+ * @param all if true, return all metrics even if unchanged.
+ */
+ @Override
+ public void getMetrics(MetricsCollector collector, boolean all) {
+ MetricsRecordBuilder metricsRecordBuilder = collector.addRecord("shuffleService");
--- End diff --
renamed
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by mareksimunek <gi...@git.apache.org>.
Github user mareksimunek commented on a diff in the pull request:
https://github.com/apache/spark/pull/22485#discussion_r220137477
--- Diff: common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleServiceMetrics.java ---
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.network.yarn;
+
+import com.codahale.metrics.*;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.spark.network.shuffle.ExternalShuffleBlockHandler;
+
+import java.util.Map;
--- End diff --
fixed
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by mareksimunek <gi...@git.apache.org>.
Github user mareksimunek commented on a diff in the pull request:
https://github.com/apache/spark/pull/22485#discussion_r220193412
--- Diff: common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleService.java ---
@@ -168,6 +170,15 @@ protected void serviceInit(Configuration conf) throws Exception {
TransportConf transportConf = new TransportConf("shuffle", new HadoopConfigProvider(conf));
blockHandler = new ExternalShuffleBlockHandler(transportConf, registeredExecutorFile);
+ // register metrics on the block handler into the Node Manager's metrics system.
+ YarnShuffleServiceMetrics serviceMetrics =
+ new YarnShuffleServiceMetrics(blockHandler.getAllMetrics());
+
+ MetricsSystemImpl metricsSystem = (MetricsSystemImpl) DefaultMetricsSystem.instance();
+ metricsSystem.register(
+ "shuffleService", "Metrics on the Spark Shuffle Service", serviceMetrics);
--- End diff --
renamed
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/22485#discussion_r219860309
--- Diff: common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleServiceMetrics.java ---
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.network.yarn;
+
+import com.codahale.metrics.*;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
--- End diff --
add a newline between hadoop and spark
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22485
**[Test build #96547 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96547/testReport)** for PR 22485 at commit [`95856e9`](https://github.com/apache/spark/commit/95856e9e464e3ec1f7786446699fb62db1c18ea2).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/22485#discussion_r219311876
--- Diff: common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleService.java ---
@@ -168,6 +170,15 @@ protected void serviceInit(Configuration conf) throws Exception {
TransportConf transportConf = new TransportConf("shuffle", new HadoopConfigProvider(conf));
blockHandler = new ExternalShuffleBlockHandler(transportConf, registeredExecutorFile);
+ // register metrics on the block handler into the Node Manager's metrics system.
+ YarnShuffleServiceMetrics serviceMetrics =
+ new YarnShuffleServiceMetrics(blockHandler.getAllMetrics());
--- End diff --
indentation should be 2 spaces, fix here and below
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22485
**[Test build #96357 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96357/testReport)** for PR 22485 at commit [`6c96397`](https://github.com/apache/spark/commit/6c96397536af57a8bbe8dd2529547427f643512b).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the issue:
https://github.com/apache/spark/pull/22485
few minor nits that I missed in my first pass, otherwise looks good.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by ash211 <gi...@git.apache.org>.
Github user ash211 commented on the issue:
https://github.com/apache/spark/pull/22485
Sorry for not following through on getting this into Apache.
FWIW, it's been in the Palantir fork of Spark for over a year: https://github.com/palantir/spark/search?q=SPARK-18364&unscoped_q=SPARK-18364
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/22485#discussion_r219890457
--- Diff: common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleServiceMetrics.java ---
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.network.yarn;
+
+import com.codahale.metrics.*;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.spark.network.shuffle.ExternalShuffleBlockHandler;
+
+import java.util.Map;
--- End diff --
nit: java includes should go before the org.apache ones
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22485
**[Test build #96552 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96552/testReport)** for PR 22485 at commit [`e259b6f`](https://github.com/apache/spark/commit/e259b6f899ab44e38309b34c58134772c2e591bf).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/22485#discussion_r219954904
--- Diff: common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleServiceMetrics.java ---
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.network.yarn;
+
+import com.codahale.metrics.*;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.spark.network.shuffle.ExternalShuffleBlockHandler;
+
+import java.util.Map;
+
+/**
+ * Forward {@link ExternalShuffleBlockHandler.ShuffleMetrics} to hadoop metrics system.
+ * NodeManager by default exposes JMX endpoint where can be collected.
+ */
+class YarnShuffleServiceMetrics implements MetricsSource {
+
+ private final MetricSet metricSet;
+
+ YarnShuffleServiceMetrics(MetricSet metricSet) {
+ this.metricSet = metricSet;
+ }
+
+ /**
+ * Get metrics from the source
+ *
+ * @param collector to contain the resulting metrics snapshot
+ * @param all if true, return all metrics even if unchanged.
+ */
+ @Override
+ public void getMetrics(MetricsCollector collector, boolean all) {
+ MetricsRecordBuilder metricsRecordBuilder = collector.addRecord("shuffleService");
+
+ for (Map.Entry<String, Metric> entry : metricSet.getMetrics().entrySet()) {
+ collectMetric(metricsRecordBuilder, entry.getKey(), entry.getValue());
+ }
+ }
+
+ /**
+ * The metric types used in {@link ExternalShuffleBlockHandler.ShuffleMetrics}
+ */
--- End diff --
add a comment to javadoc that its visible for testing
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the issue:
https://github.com/apache/spark/pull/22485
test this please
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22485
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22485
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/22485#discussion_r219506036
--- Diff: resource-managers/yarn/src/test/scala/org/apache/spark/network/yarn/YarnShuffleServiceMetricsSuite.scala ---
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.network.yarn
+
+import scala.collection.JavaConverters._
+
+import org.apache.hadoop.metrics2.MetricsRecordBuilder
+import org.mockito.Matchers._
+import org.mockito.Mockito.{mock, times, verify, when}
+import org.scalatest.Matchers
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.network.server.OneForOneStreamManager
+import org.apache.spark.network.shuffle.{ExternalShuffleBlockHandler, ExternalShuffleBlockResolver}
+
+class YarnShuffleServiceMetricsSuite extends SparkFunSuite with Matchers {
+
+ val streamManager = mock(classOf[OneForOneStreamManager])
+ val blockResolver = mock(classOf[ExternalShuffleBlockResolver])
+ when(blockResolver.getRegisteredExecutorsSize).thenReturn(42)
+
+ val metrics = new ExternalShuffleBlockHandler(streamManager, blockResolver).getAllMetrics
+
+ test("metrics named as expected") {
+ val allMetrics = Set(
+ "openBlockRequestLatencyMillis", "registerExecutorRequestLatencyMillis",
+ "blockTransferRateBytes", "registeredExecutorsSize")
+
+ metrics.getMetrics.keySet().asScala should be (allMetrics)
+ }
+
+ // these three metrics have the same effect on the collector
+ for (testname <- Seq("openBlockRequestLatencyMillis",
+ "registerExecutorRequestLatencyMillis",
+ "blockTransferRateBytes")) {
+ test(s"$testname - collector receives correct types") {
+ val builder = mock(classOf[MetricsRecordBuilder])
+ when(builder.addCounter(any(), anyLong())).thenReturn(builder)
--- End diff --
I don't see where these when's are used?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by mareksimunek <gi...@git.apache.org>.
Github user mareksimunek commented on a diff in the pull request:
https://github.com/apache/spark/pull/22485#discussion_r220137451
--- Diff: common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleServiceMetrics.java ---
@@ -56,7 +56,7 @@ public void getMetrics(MetricsCollector collector, boolean all) {
/**
* The metric types used in {@link ExternalShuffleBlockHandler.ShuffleMetrics}
*/
- private static void collectMetric(MetricsRecordBuilder metricsRecordBuilder, String name, Metric metric) {
--- End diff --
added
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22485
**[Test build #96547 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96547/testReport)** for PR 22485 at commit [`95856e9`](https://github.com/apache/spark/commit/95856e9e464e3ec1f7786446699fb62db1c18ea2).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the issue:
https://github.com/apache/spark/pull/22485
@ash211 thanks for commenting, looks like you added https://github.com/palantir/spark/pull/236 on top of what is here. looks like it gets snapshot of the timer values to return some of the raw values and percentiles rather then just some rates, is that correct?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22485
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/22485#discussion_r219954776
--- Diff: common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleServiceMetrics.java ---
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.network.yarn;
+
+import com.codahale.metrics.*;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsRecordBuilder;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.spark.network.shuffle.ExternalShuffleBlockHandler;
+
+import java.util.Map;
+
+/**
+ * Forward {@link ExternalShuffleBlockHandler.ShuffleMetrics} to hadoop metrics system.
+ * NodeManager by default exposes JMX endpoint where can be collected.
+ */
+class YarnShuffleServiceMetrics implements MetricsSource {
+
+ private final MetricSet metricSet;
+
+ YarnShuffleServiceMetrics(MetricSet metricSet) {
+ this.metricSet = metricSet;
+ }
+
+ /**
+ * Get metrics from the source
+ *
+ * @param collector to contain the resulting metrics snapshot
+ * @param all if true, return all metrics even if unchanged.
+ */
+ @Override
+ public void getMetrics(MetricsCollector collector, boolean all) {
+ MetricsRecordBuilder metricsRecordBuilder = collector.addRecord("shuffleService");
--- End diff --
I think we should rename this to be sparkShuffleService in order to differentiate from other shuffle servers that could be on the nodemanager (MR/tez).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/22485#discussion_r219508486
--- Diff: resource-managers/yarn/src/test/scala/org/apache/spark/network/yarn/YarnShuffleServiceMetricsSuite.scala ---
@@ -0,0 +1,75 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.spark.network.yarn
+
+import scala.collection.JavaConverters._
+
+import org.apache.hadoop.metrics2.MetricsRecordBuilder
+import org.mockito.Matchers._
+import org.mockito.Mockito.{mock, times, verify, when}
+import org.scalatest.Matchers
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.network.server.OneForOneStreamManager
+import org.apache.spark.network.shuffle.{ExternalShuffleBlockHandler, ExternalShuffleBlockResolver}
+
+class YarnShuffleServiceMetricsSuite extends SparkFunSuite with Matchers {
+
+ val streamManager = mock(classOf[OneForOneStreamManager])
+ val blockResolver = mock(classOf[ExternalShuffleBlockResolver])
+ when(blockResolver.getRegisteredExecutorsSize).thenReturn(42)
+
+ val metrics = new ExternalShuffleBlockHandler(streamManager, blockResolver).getAllMetrics
+
+ test("metrics named as expected") {
+ val allMetrics = Set(
+ "openBlockRequestLatencyMillis", "registerExecutorRequestLatencyMillis",
+ "blockTransferRateBytes", "registeredExecutorsSize")
+
+ metrics.getMetrics.keySet().asScala should be (allMetrics)
+ }
+
+ // these three metrics have the same effect on the collector
+ for (testname <- Seq("openBlockRequestLatencyMillis",
+ "registerExecutorRequestLatencyMillis",
+ "blockTransferRateBytes")) {
+ test(s"$testname - collector receives correct types") {
+ val builder = mock(classOf[MetricsRecordBuilder])
+ when(builder.addCounter(any(), anyLong())).thenReturn(builder)
+ when(builder.addGauge(any(), anyDouble())).thenReturn(builder)
+
+ YarnShuffleServiceMetrics.collectMetric(builder, testname,
--- End diff --
you are calling a private function here
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffl...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/22485
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22485
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22485
**[Test build #96516 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96516/testReport)** for PR 22485 at commit [`764404a`](https://github.com/apache/spark/commit/764404a89eacc58898a3fff2e49b00ac2c1a01b3).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22485: [SPARK-18364][YARN] Expose metrics for YarnShuffleServic...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22485
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96516/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org