Skip to content

YARN-11122. Support getClusterNodes API in FederationClientInterceptor #4274

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
goiri merged 17 commits into from
May 15, 2022
Merged

YARN-11122. Support getClusterNodes API in FederationClientInterceptor #4274

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
7bd9675
YARN-11122:Implement the method of getting ClusterNode, including rel…
May 2, 2022
2af73f5
YARN-11122:
May 7, 2022
87277d1
YARN-11122:Fix CheckStyle
May 7, 2022
6630ce6
YARN-11122:Fix CheckStyle
May 7, 2022
07f6090
YARN-11122. Fix CheckStyle.
May 10, 2022
1bf7332
YARN-11122. Fix CheckStyle
May 10, 2022
b4867c0
YARN-11122: Fix Code Style, indentation
May 12, 2022
aa49c3a
YARN-11122: Add GetClusterNodes Metric Junit Test.
May 12, 2022
c6e404b
YARN-11122: Fix CodeStyle.
May 12, 2022
c7ccca5
YARN-11122: Fix CodeStyle.
May 12, 2022
407c039
YARN-11122: Fix CodeStyle.
May 12, 2022
7845199
YARN-11122: Fix CodeStyle.
May 12, 2022
f16651f
YARN-11122: Fix CodeStyle.
May 12, 2022
dde385a
YARN-11122: Fix CodeStyle.
May 12, 2022
72359e0
YARN-11122. Fix CodeStyle.
May 12, 2022
0887df5
YARN-11122. Fix CodeStyle
May 13, 2022
dbd5cb3
YARN-11122. Fix CodeStyle
May 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion ...-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/RouterMetrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ public final class RouterMetrics {
private MutableGaugeInt numAppAttemptsFailedRetrieved;
@Metric("# of getClusterMetrics failed to be retrieved")
private MutableGaugeInt numGetClusterMetricsFailedRetrieved;
@Metric("# of getClusterNodes failed to be retrieved")
private MutableGaugeInt numGetClusterNodesFailedRetrieved;

// Aggregate metrics are shared, and don't have to be looked up per call
@Metric("Total number of successful Submitted apps and latency(ms)")
Expand All @@ -74,7 +76,8 @@ public final class RouterMetrics {
@Metric("Total number of successful Retrieved getClusterMetrics and "
+ "latency(ms)")
private MutableRate totalSucceededGetClusterMetricsRetrieved;

@Metric("Total number of successful Retrieved getClusterNodes and latency(ms)")
private MutableRate totalSucceededGetClusterNodesRetrieved;

/**
* Provide quantile counters for all latencies.
Expand All @@ -86,6 +89,7 @@ public final class RouterMetrics {
private MutableQuantiles getApplicationsReportLatency;
private MutableQuantiles getApplicationAttemptReportLatency;
private MutableQuantiles getClusterMetricsLatency;
private MutableQuantiles getClusterNodesLatency;

private static volatile RouterMetrics INSTANCE = null;
private static MetricsRegistry registry;
Expand All @@ -112,6 +116,10 @@ private RouterMetrics() {
getClusterMetricsLatency =
registry.newQuantiles("getClusterMetricsLatency",
"latency of get cluster metrics", "ops", "latency", 10);

getClusterNodesLatency =
registry.newQuantiles("getClusterNodesLatency",
Copy link
Member

@goiri goiri May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yetus seems not to be running the checkstyle.
Can you run it by hand and fix indentation?

CC @ayushtkn any idea on why the checkstyle is not being ran?

Copy link
Contributor Author

@slfan1989 slfan1989 May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no problem, I'll fix the indentation.

Copy link
Member

@ayushtkn ayushtkn May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It did ran @goiri for the latest 14th build, for the 5th the results aren't there now
Branch:
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4274/14/artifact/out/buildtool-branch-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router.txt
Patch:
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4274/14/artifact/out/buildtool-patch-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-router.txt

Both have same number hence 0

Copy link
Member

@goiri goiri May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, the Yetus report is not showing and is giving a +1.

"latency of get cluster nodes", "ops", "latency", 10);
}

public static RouterMetrics getMetrics() {
Expand Down Expand Up @@ -168,6 +176,11 @@ public long getNumSucceededGetClusterMetricsRetrieved(){
return totalSucceededGetClusterMetricsRetrieved.lastStat().numSamples();
}

@VisibleForTesting
public long getNumSucceededGetClusterNodesRetrieved(){
return totalSucceededGetClusterNodesRetrieved.lastStat().numSamples();
}

@VisibleForTesting
public double getLatencySucceededAppsCreated() {
return totalSucceededAppsCreated.lastStat().mean();
Expand Down Expand Up @@ -203,6 +216,11 @@ public double getLatencySucceededGetClusterMetricsRetrieved() {
return totalSucceededGetClusterMetricsRetrieved.lastStat().mean();
}

@VisibleForTesting
public double getLatencySucceededGetClusterNodesRetrieved() {
return totalSucceededGetClusterNodesRetrieved.lastStat().mean();
}

@VisibleForTesting
public int getAppsFailedCreated() {
return numAppsFailedCreated.value();
Expand Down Expand Up @@ -273,6 +291,11 @@ public void succeededGetClusterMetricsRetrieved(long duration) {
getClusterMetricsLatency.add(duration);
}

public void succeededGetClusterNodesRetrieved(long duration) {
totalSucceededGetClusterNodesRetrieved.add(duration);
getClusterNodesLatency.add(duration);
}

public void incrAppsFailedCreated() {
numAppsFailedCreated.incr();
}
Expand Down Expand Up @@ -301,4 +324,7 @@ public void incrGetClusterMetricsFailedRetrieved() {
numGetClusterMetricsFailedRetrieved.incr();
}

public void incrClusterNodesFailedRetrieved() {
numGetClusterNodesFailedRetrieved.incr();
}
}
29 changes: 27 additions & 2 deletions .../main/java/org/apache/hadoop/yarn/server/router/clientrm/FederationClientInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

package org.apache.hadoop.yarn.server.router.clientrm;

import org.apache.hadoop.thirdparty.com.google.common.collect.Maps;
import org.apache.hadoop.thirdparty.com.google.common.util.concurrent.ThreadFactoryBuilder;
import java.io.IOException;
import java.lang.reflect.Method;
Expand Down Expand Up @@ -790,8 +791,32 @@ <R> Map<SubClusterId, R> invokeConcurrent(Collection<SubClusterId> clusterIds,

@Override
public GetClusterNodesResponse getClusterNodes(GetClusterNodesRequest request)
throws YarnException, IOException {
throw new NotImplementedException("Code is not implemented");
throws YarnException, IOException {
Copy link
Member

@goiri goiri May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation

Copy link
Contributor Author

@slfan1989 slfan1989 May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I will fix it

if (request == null) {
routerMetrics.incrClusterNodesFailedRetrieved();
RouterServerUtil.logAndThrowException(
Copy link
Member

@goiri goiri May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Single line.

Copy link
Contributor Author

@slfan1989 slfan1989 May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I will fix it.

"Missing getClusterNodes request.", null);
}
long startTime = clock.getTime();
Map<SubClusterId, SubClusterInfo> subclusters =
federationFacade.getSubClusters(true);
Map<SubClusterId, GetClusterNodesResponse> clusterNodes = Maps.newHashMap();
for (Map.Entry<SubClusterId, SubClusterInfo> entry : subclusters.entrySet()) {
Copy link
Member

@goiri goiri May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't use the values. We shouldn't use entrySet() but keys()

Copy link
Contributor Author

@slfan1989 slfan1989 May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you, good advice, keysets should be used.

SubClusterId subClusterId = entry.getKey();
ApplicationClientProtocol client = null;
try {
client = getClientRMProxyForSubCluster(subClusterId);
GetClusterNodesResponse response = client.getClusterNodes(request);
clusterNodes.put(subClusterId, response);
} catch (Exception ex) {
routerMetrics.incrClusterNodesFailedRetrieved();
LOG.error("Unable to get cluster nodes due to exception.", ex);
}
}
long stopTime = clock.getTime();
routerMetrics.succeededGetClusterNodesRetrieved(stopTime - startTime);
// Merge the NodesResponse
return RouterYarnClientUtils.mergeClusterNodesResponse(clusterNodes.values());
}

@Override
Expand Down
27 changes: 27 additions & 0 deletions ...er/src/main/java/org/apache/hadoop/yarn/server/router/clientrm/RouterYarnClientUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,19 @@
import java.util.Collection;
import java.util.HashMap;
import java.util.Map;
import java.util.List;
import java.util.ArrayList;

import org.apache.hadoop.yarn.api.protocolrecords.GetApplicationsResponse;
import org.apache.hadoop.yarn.api.protocolrecords.GetClusterMetricsResponse;
import org.apache.hadoop.yarn.api.protocolrecords.GetClusterNodesResponse;
import org.apache.hadoop.yarn.api.records.ApplicationId;
import org.apache.hadoop.yarn.api.records.ApplicationReport;
import org.apache.hadoop.yarn.api.records.ApplicationResourceUsageReport;
import org.apache.hadoop.yarn.api.records.YarnClusterMetrics;
import org.apache.hadoop.yarn.api.records.NodeReport;
import org.apache.hadoop.yarn.server.uam.UnmanagedApplicationManager;
import org.apache.hadoop.yarn.util.Records;
import org.apache.hadoop.yarn.util.resource.Resources;

/**
Expand Down Expand Up @@ -194,4 +199,26 @@ private static boolean mergeUamToReport(String appName,
return !(appName.startsWith(UnmanagedApplicationManager.APP_NAME) ||
appName.startsWith(PARTIAL_REPORT));
}

/**
* Merges a list of GetClusterNodesResponse.
*
* @param responses a list of GetClusterNodesResponse to merge.
* @return the merged GetClusterNodesResponse.
*/
public static GetClusterNodesResponse mergeClusterNodesResponse(
Collection<GetClusterNodesResponse> responses) {

GetClusterNodesResponse clusterNodesResponse = Records.newRecord(GetClusterNodesResponse.class);

List<NodeReport> nodeReports = new ArrayList<>();

for (GetClusterNodesResponse response : responses) {
if (response != null && response.getNodeReports() != null) {
nodeReports.addAll(response.getNodeReports());
}
}
clusterNodesResponse.setNodeReports(nodeReports);
return clusterNodesResponse;
}
}
16 changes: 16 additions & 0 deletions ...t/java/org/apache/hadoop/yarn/server/router/clientrm/TestFederationClientInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@
import org.apache.hadoop.yarn.api.protocolrecords.KillApplicationResponse;
import org.apache.hadoop.yarn.api.protocolrecords.SubmitApplicationRequest;
import org.apache.hadoop.yarn.api.protocolrecords.SubmitApplicationResponse;
import org.apache.hadoop.yarn.api.protocolrecords.GetClusterNodesResponse;
import org.apache.hadoop.yarn.api.protocolrecords.GetClusterNodesRequest;
import org.apache.hadoop.yarn.api.records.ApplicationAttemptId;
import org.apache.hadoop.yarn.api.records.ApplicationId;
import org.apache.hadoop.yarn.api.records.ApplicationSubmissionContext;
Expand Down Expand Up @@ -641,4 +643,18 @@ public void testGetApplicationsApplicationStateNotExists() throws Exception{
Assert.assertNotNull(responseGet);
Assert.assertTrue(responseGet.getApplicationList().isEmpty());
}

@Test
public void testGetClusterNodesRequest() throws Exception {
LOG.info("Test FederationClientInterceptor : Get Cluster Nodeds request");
// null request
LambdaTestUtils.intercept(YarnException.class,
"Missing getClusterNodes request.", () -> interceptor.getClusterNodes(null));

// normal request.
GetClusterNodesResponse response =
interceptor.getClusterNodes(GetClusterNodesRequest.newInstance());
Assert.assertEquals(subClusters.size(),
response.getNodeReports().size());
}
}