[GitHub] [hadoop] goiri commented on a diff in pull request #4738: YARN-11250. Capture the Performance Metrics of ZookeeperFederationStateStore.
goiri commented on code in PR #4738:
URL: https://github.com/apache/hadoop/pull/4738#discussion_r948369273
##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/store/impl/ZKFederationStateStoreOpDurations.java:
##
@@ -0,0 +1,155 @@
+/**
+ * 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.hadoop.yarn.server.federation.store.impl;
+
+import org.apache.hadoop.classification.InterfaceAudience;
+import org.apache.hadoop.classification.InterfaceStability;
+import org.apache.hadoop.metrics2.MetricsCollector;
+import org.apache.hadoop.metrics2.MetricsInfo;
+import org.apache.hadoop.metrics2.MetricsSource;
+import org.apache.hadoop.metrics2.MetricsSystem;
+import org.apache.hadoop.metrics2.annotation.Metric;
+import org.apache.hadoop.metrics2.annotation.Metrics;
+import org.apache.hadoop.metrics2.lib.DefaultMetricsSystem;
+import org.apache.hadoop.metrics2.lib.MetricsRegistry;
+import org.apache.hadoop.metrics2.lib.MutableRate;
+
+import static org.apache.hadoop.metrics2.lib.Interns.info;
+
+@InterfaceAudience.Private
+@InterfaceStability.Unstable
+@Metrics(context="ZKFederationStateStore-op-durations")
+public final class ZKFederationStateStoreOpDurations implements MetricsSource {
+
+ @Metric("Duration for a add application homeSubcluster call")
+ private MutableRate addAppHomeSubCluster;
+
+ @Metric("Duration for a update application homeSubcluster call")
+ private MutableRate updateAppHomeSubCluster;
+
+ @Metric("Duration for a get application homeSubcluster call")
+ private MutableRate getAppHomeSubCluster;
+
+ @Metric("Duration for a get applications homeSubcluster call")
+ private MutableRate getAppsHomeSubCluster;
+
+ @Metric("Duration for a delete applications homeSubcluster call")
+ private MutableRate deleteAppHomeSubCluster;
+
+ @Metric("Duration for a register subCluster call")
+ private MutableRate registerSubCluster;
+
+ @Metric("Duration for a deregister subCluster call")
+ private MutableRate deregisterSubCluster;
+
+ @Metric("Duration for a subCluster Heartbeat call")
+ private MutableRate subClusterHeartbeat;
+
+ @Metric("Duration for a get SubCluster call")
+ private MutableRate getSubCluster;
+
+ @Metric("Duration for a get SubClusters call")
+ private MutableRate getSubClusters;
+
+ @Metric("Duration for a get PolicyConfiguration call")
+ private MutableRate getPolicyConfiguration;
+
+ @Metric("Duration for a set PolicyConfiguration call")
+ private MutableRate setPolicyConfiguration;
+
+ @Metric("Duration for a get PolicyConfigurations call")
+ private MutableRate getPoliciesConfigurations;
+
+ protected static final MetricsInfo RECORD_INFO =
+ info("ZKFederationStateStoreOpDurations", "Durations of
ZKFederationStateStore calls");
+
+ private final MetricsRegistry registry;
+
+ private static final ZKFederationStateStoreOpDurations INSTANCE =
+ new ZKFederationStateStoreOpDurations();
+
+ public static ZKFederationStateStoreOpDurations getInstance() {
+return INSTANCE;
+ }
+
+ private ZKFederationStateStoreOpDurations() {
+registry = new MetricsRegistry(RECORD_INFO);
+registry.tag(RECORD_INFO, "ZKFederationStateStoreOpDurations");
+
+MetricsSystem ms = DefaultMetricsSystem.instance();
+if (ms != null) {
+ ms.register(RECORD_INFO.name(), RECORD_INFO.description(), this);
+}
+ }
+
+ @Override
+ public synchronized void getMetrics(MetricsCollector collector, boolean all)
{
+registry.snapshot(collector.addRecord(registry.info()), all);
+ }
+
+ public void addAppHomeSubClusterDuration(long endTime, long startTime) {
+addAppHomeSubCluster.add(endTime - startTime);
+ }
+
+ public void addUpdateAppHomeSubClusterDuration(long endTime, long startTime)
{
+updateAppHomeSubCluster.add(endTime - startTime);
+ }
+
+ public void addGetAppHomeSubClusterDuration(long endTime, long startTime) {
+getAppHomeSubCluster.add(endTime - startTime);
+ }
+
+ public void addGetAppsHomeSubClusterDuration(long endTime, long startTime) {
+getAppsHomeSubCluster.add(endTime - startTime);
+ }
+
+ public void addDeleteAppHomeSubClusterDuration(long
[GitHub] [hadoop] goiri commented on a diff in pull request #4738: YARN-11250. Capture the Performance Metrics of ZookeeperFederationStateStore.
goiri commented on code in PR #4738:
URL: https://github.com/apache/hadoop/pull/4738#discussion_r948194089
##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/store/impl/ZookeeperFederationStateStore.java:
##
@@ -184,16 +193,17 @@ public AddApplicationHomeSubClusterResponse
addApplicationHomeSubCluster(
FederationStateStoreUtils.logAndThrowStoreException(LOG, errMsg);
}
-return AddApplicationHomeSubClusterResponse
-.newInstance(homeSubCluster);
+opDurations.addAppHomeSubClusterDuration(clock.getTime() - start);
Review Comment:
No need to do this but one thing I was thinking about is the fact that we
keep doing the substraction in the call.
Would it look better to pass the start and the end time and do the
substraction inside?
##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/store/impl/ZookeeperFederationStateStore.java:
##
@@ -161,8 +169,9 @@ public void close() throws Exception {
@Override
public AddApplicationHomeSubClusterResponse addApplicationHomeSubCluster(
- AddApplicationHomeSubClusterRequest request) throws YarnException {
+ AddApplicationHomeSubClusterRequest request) throws YarnException {
Review Comment:
The old indentation was correct.
##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/store/impl/ZookeeperFederationStateStore.java:
##
@@ -336,14 +354,14 @@ public SubClusterHeartbeatResponse subClusterHeartbeat(
subClusterInfo.setCapability(request.getCapability());
putSubclusterInfo(subClusterId, subClusterInfo, true);
-
+opDurations.addSubClusterHeartbeatDuration(clock.getTime() - start);
return SubClusterHeartbeatResponse.newInstance();
}
@Override
public GetSubClusterInfoResponse getSubCluster(
- GetSubClusterInfoRequest request) throws YarnException {
-
+ GetSubClusterInfoRequest request) throws YarnException {
Review Comment:
Indentation all over.
##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/store/impl/ZookeeperFederationStateStore.java:
##
@@ -203,53 +213,58 @@ public AddApplicationHomeSubClusterResponse
addApplicationHomeSubCluster(
FederationStateStoreUtils.logAndThrowStoreException(LOG, errMsg);
}
SubClusterId newSubClusterId =
-request.getApplicationHomeSubCluster().getHomeSubCluster();
+ request.getApplicationHomeSubCluster().getHomeSubCluster();
putApp(appId, newSubClusterId, true);
+
+opDurations.addUpdateAppHomeSubClusterDuration(clock.getTime() - start);
return UpdateApplicationHomeSubClusterResponse.newInstance();
}
@Override
public GetApplicationHomeSubClusterResponse getApplicationHomeSubCluster(
- GetApplicationHomeSubClusterRequest request) throws YarnException {
+ GetApplicationHomeSubClusterRequest request) throws YarnException {
+long start = clock.getTime();
FederationApplicationHomeSubClusterStoreInputValidator.validate(request);
ApplicationId appId = request.getApplicationId();
SubClusterId homeSubCluster = getApp(appId);
if (homeSubCluster == null) {
String errMsg = "Application " + appId + " does not exist";
FederationStateStoreUtils.logAndThrowStoreException(LOG, errMsg);
}
+opDurations.addGetAppHomeSubClusterDuration(clock.getTime() - start);
return GetApplicationHomeSubClusterResponse.newInstance(
-ApplicationHomeSubCluster.newInstance(appId, homeSubCluster));
+ApplicationHomeSubCluster.newInstance(appId, homeSubCluster));
}
@Override
public GetApplicationsHomeSubClusterResponse getApplicationsHomeSubCluster(
- GetApplicationsHomeSubClusterRequest request) throws YarnException {
+ GetApplicationsHomeSubClusterRequest request) throws YarnException {
Review Comment:
Indetation
##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/store/impl/ZookeeperFederationStateStore.java:
##
@@ -203,53 +213,58 @@ public AddApplicationHomeSubClusterResponse
addApplicationHomeSubCluster(
FederationStateStoreUtils.logAndThrowStoreException(LOG, errMsg);
}
SubClusterId newSubClusterId =
-request.getApplicationHomeSubCluster().getHomeSubCluster();
+ request.getApplicationHomeSubCluster().getHomeSubCluster();
putApp(appId, newSubClusterId, true);
+
+opDurations.addUpdateAppHomeSubClusterDuration(clock.getTime() - start);
return UpdateApplicationHomeSubClusterResponse.newInstance();
}
@Override
public
[GitHub] [hadoop] goiri commented on a diff in pull request #4738: YARN-11250. Capture the Performance Metrics of ZookeeperFederationStateStore.
goiri commented on code in PR #4738:
URL: https://github.com/apache/hadoop/pull/4738#discussion_r947214942
##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/store/impl/ZookeeperFederationStateStore.java:
##
@@ -104,29 +107,39 @@
public class ZookeeperFederationStateStore implements FederationStateStore {
private static final Logger LOG =
- LoggerFactory.getLogger(ZookeeperFederationStateStore.class);
+ LoggerFactory.getLogger(ZookeeperFederationStateStore.class);
Review Comment:
Avoid
##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/store/impl/ZookeeperFederationStateStore.java:
##
@@ -104,29 +107,39 @@
public class ZookeeperFederationStateStore implements FederationStateStore {
private static final Logger LOG =
- LoggerFactory.getLogger(ZookeeperFederationStateStore.class);
+ LoggerFactory.getLogger(ZookeeperFederationStateStore.class);
private final static String ROOT_ZNODE_NAME_MEMBERSHIP = "memberships";
private final static String ROOT_ZNODE_NAME_APPLICATION = "applications";
private final static String ROOT_ZNODE_NAME_POLICY = "policies";
- /** Interface to Zookeeper. */
+ /**
+ * Interface to Zookeeper.
Review Comment:
The old format is common; I would leave it as is
##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/store/impl/ZookeeperFederationStateStore.java:
##
@@ -203,53 +217,58 @@ public AddApplicationHomeSubClusterResponse
addApplicationHomeSubCluster(
FederationStateStoreUtils.logAndThrowStoreException(LOG, errMsg);
}
SubClusterId newSubClusterId =
-request.getApplicationHomeSubCluster().getHomeSubCluster();
+request.getApplicationHomeSubCluster().getHomeSubCluster();
Review Comment:
This new spacing is wrong.
##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/store/impl/ZookeeperFederationStateStore.java:
##
@@ -104,29 +107,39 @@
public class ZookeeperFederationStateStore implements FederationStateStore {
private static final Logger LOG =
- LoggerFactory.getLogger(ZookeeperFederationStateStore.class);
+ LoggerFactory.getLogger(ZookeeperFederationStateStore.class);
private final static String ROOT_ZNODE_NAME_MEMBERSHIP = "memberships";
private final static String ROOT_ZNODE_NAME_APPLICATION = "applications";
private final static String ROOT_ZNODE_NAME_POLICY = "policies";
- /** Interface to Zookeeper. */
+ /**
+ * Interface to Zookeeper.
+ */
private ZKCuratorManager zkManager;
- /** Directory to store the state store data. */
+ /**
+ * Directory to store the state store data.
+ */
private String baseZNode;
private String appsZNode;
private String membershipZNode;
private String policiesZNode;
+ private volatile Clock clock = SystemClock.getInstance();
+
+ @VisibleForTesting
+ private ZKFederationStateStoreOpDurations opDurations =
+ ZKFederationStateStoreOpDurations.getInstance();
+
@Override
public void init(Configuration conf) throws YarnException {
LOG.info("Initializing ZooKeeper connection");
baseZNode = conf.get(
-YarnConfiguration.FEDERATION_STATESTORE_ZK_PARENT_PATH,
-YarnConfiguration.DEFAULT_FEDERATION_STATESTORE_ZK_PARENT_PATH);
+YarnConfiguration.FEDERATION_STATESTORE_ZK_PARENT_PATH,
Review Comment:
This was fine before
--
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: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
[GitHub] [hadoop] goiri commented on a diff in pull request #4738: YARN-11250. Capture the Performance Metrics of ZookeeperFederationStateStore.
goiri commented on code in PR #4738:
URL: https://github.com/apache/hadoop/pull/4738#discussion_r946118671
##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/store/impl/ZKFederationStateStoreOpDurations.java:
##
@@ -54,25 +54,25 @@ public final class ZKFederationStateStoreOpDurations
implements MetricsSource {
private MutableRate registerSubClusterCall;
@Metric("Duration for a deregister subCluster call")
- private MutableRate deregisterSubCluster;
+ private MutableRate deregisterSubClusterCall;
Review Comment:
Do we need to add the Call at the end?
--
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: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
