slfan1989 commented on code in PR #4618:
URL: https://github.com/apache/hadoop/pull/4618#discussion_r929581152
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/clientrm/FederationClientInterceptor.java:
##########
@@ -280,47 +274,50 @@ private SubClusterId getRandomActiveSubCluster(
public GetNewApplicationResponse getNewApplication(
GetNewApplicationRequest request) throws YarnException, IOException {
- long startTime = clock.getTime();
+ if(request == null) {
+ routerMetrics.incrAppsFailedCreated();
+ String errMsg = "Missing getNewApplication request.";
+ RouterAuditLogger.logFailure(user.getShortUserName(),
+ RouterAuditLogger.AuditConstants.GET_NEW_APP, "UNKNOWN",
+ "RouterClientRMService", errMsg);
+ RouterServerUtil.logAndThrowException(errMsg, null);
+ }
+ long startTime = clock.getTime();
Map<SubClusterId, SubClusterInfo> subClustersActive =
federationFacade.getSubClusters(true);
+ GetNewApplicationResponse response = null;
+
for (int i = 0; i < numSubmitRetries; ++i) {
SubClusterId subClusterId = getRandomActiveSubCluster(subClustersActive);
- LOG.debug(
- "getNewApplication try #{} on SubCluster {}", i, subClusterId);
- ApplicationClientProtocol clientRMProxy =
- getClientRMProxyForSubCluster(subClusterId);
- GetNewApplicationResponse response = null;
+ LOG.info("getNewApplication try #{} on SubCluster {}", i, subClusterId);
+ ApplicationClientProtocol clientRMProxy =
getClientRMProxyForSubCluster(subClusterId);
+ response = null;
try {
response = clientRMProxy.getNewApplication(request);
} catch (Exception e) {
- LOG.warn("Unable to create a new ApplicationId in SubCluster "
- + subClusterId.getId(), e);
+ LOG.warn("Unable to create a new ApplicationId in SubCluster {}.",
subClusterId.getId(), e);
+ subClustersActive.remove(subClusterId);
}
if (response != null) {
-
long stopTime = clock.getTime();
routerMetrics.succeededAppsCreated(stopTime - startTime);
RouterAuditLogger.logSuccess(user.getShortUserName(),
RouterAuditLogger.AuditConstants.GET_NEW_APP,
"RouterClientRMService", response.getApplicationId());
return response;
- } else {
- // Empty response from the ResourceManager.
- // Blacklist this subcluster for this request.
- subClustersActive.remove(subClusterId);
Review Comment:
When reading this code, I found that response == null only when the
exception is thrown , I think this logic can be moved to the previous try...
catch
```
try {
response = clientRMProxy.getNewApplication(request);
} catch (Exception e) {
// When an exception occurs, response == null
LOG.warn("Unable to create a new ApplicationId in SubCluster "
+ subClusterId.getId(), e);
}
```
The comment of this logic makes me think that there is a data structure
called blacklist, but there is no blacklist in the code, I think the operation
of removing subclusterId can be moved up.
```
// Empty response from the ResourceManager.
// Blacklist this subcluster for this request.
subClustersActive.remove(subClusterId);
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
