goiri commented on code in PR #4650:
URL: https://github.com/apache/hadoop/pull/4650#discussion_r932561002
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/amrmproxy/AMRMProxyService.java:
##########
@@ -486,18 +495,16 @@ protected void initializePipeline(ApplicationAttemptId
applicationAttemptId,
this.nmContext.getNMStateStore()
.removeAMRMProxyAppContext(applicationAttemptId);
} catch (IOException e) {
Review Comment:
as we are changing, might be good to call it `ioe`
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/amrmproxy/AMRMProxyService.java:
##########
@@ -588,74 +592,83 @@ protected void stopApplication(ApplicationId
applicationId) {
this.nmContext.getNMStateStore()
.removeAMRMProxyAppContext(pipeline.getApplicationAttemptId());
} catch (IOException e) {
- LOG.error("Error removing AMRMProxy application context for "
- + applicationId, e);
+ LOG.error("Error removing AMRMProxy application context for {}.",
+ applicationId, e);
+ isStopSuccess = false;
}
}
}
+
+ if(isStopSuccess) {
+ long endTime = clock.getTime();
+ this.metrics.succeededAppStopRequests(endTime - startTime);
+ } else {
+ this.metrics.incrFailedAppStopRequests();
+ }
}
private void updateAMRMTokens(AMRMTokenIdentifier amrmTokenIdentifier,
RequestInterceptorChainWrapper pipeline,
AllocateResponse allocateResponse) {
+
AMRMProxyApplicationContextImpl context =
- (AMRMProxyApplicationContextImpl) pipeline.getRootInterceptor()
- .getApplicationContext();
+ (AMRMProxyApplicationContextImpl)
pipeline.getRootInterceptor().getApplicationContext();
+
+ try {
+ long startTime = clock.getTime();
- // check to see if the RM has issued a new AMRMToken & accordingly update
- // the real ARMRMToken in the current context
- if (allocateResponse.getAMRMToken() != null) {
- LOG.info("RM rolled master-key for amrm-tokens");
+ // check to see if the RM has issued a new AMRMToken & accordingly update
+ // the real ARMRMToken in the current context
+ if (allocateResponse.getAMRMToken() != null) {
+ LOG.info("RM rolled master-key for amrm-tokens.");
- org.apache.hadoop.yarn.api.records.Token token =
- allocateResponse.getAMRMToken();
+ org.apache.hadoop.yarn.api.records.Token token =
+ allocateResponse.getAMRMToken();
- // Do not propagate this info back to AM
- allocateResponse.setAMRMToken(null);
+ // Do not propagate this info back to AM
+ allocateResponse.setAMRMToken(null);
- org.apache.hadoop.security.token.Token<AMRMTokenIdentifier> newToken =
- ConverterUtils.convertFromYarn(token, (Text) null);
+ org.apache.hadoop.security.token.Token<AMRMTokenIdentifier> newToken =
+ ConverterUtils.convertFromYarn(token, (Text) null);
- // Update the AMRMToken in context map, and in NM state store if it is
- // different
- if (context.setAMRMToken(newToken)
- && this.nmContext.getNMStateStore() != null) {
- try {
+ // Update the AMRMToken in context map, and in NM state store if it is
+ // different
+ if (context.setAMRMToken(newToken)
+ && this.nmContext.getNMStateStore() != null) {
this.nmContext.getNMStateStore().storeAMRMProxyAppContextEntry(
context.getApplicationAttemptId(), NMSS_AMRMTOKEN_KEY,
- newToken.encodeToUrlString().getBytes("UTF-8"));
- } catch (IOException e) {
- LOG.error("Error storing AMRMProxy application context entry for "
- + context.getApplicationAttemptId(), e);
+ newToken.encodeToUrlString().getBytes(StandardCharsets.UTF_8));
}
}
- }
- // Check if the local AMRMToken is rolled up and update the context and
- // response accordingly
- MasterKeyData nextMasterKey =
- this.secretManager.getNextMasterKeyData();
-
- if (nextMasterKey != null
- && nextMasterKey.getMasterKey().getKeyId() != amrmTokenIdentifier
- .getKeyId()) {
- Token<AMRMTokenIdentifier> localToken = context.getLocalAMRMToken();
- if (nextMasterKey.getMasterKey().getKeyId() != context
- .getLocalAMRMTokenKeyId()) {
- LOG.info("The local AMRMToken has been rolled-over."
- + " Send new local AMRMToken back to application: "
- + pipeline.getApplicationId());
- localToken =
- this.secretManager.createAndGetAMRMToken(pipeline
- .getApplicationAttemptId());
- context.setLocalAMRMToken(localToken);
- }
+ // Check if the local AMRMToken is rolled up and update the context and
+ // response accordingly
+ MasterKeyData nextMasterKey = this.secretManager.getNextMasterKeyData();
+
+ if (nextMasterKey != null
Review Comment:
This indentation is weird. It might be cleaner to extract some too.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/amrmproxy/AMRMProxyService.java:
##########
@@ -111,6 +113,9 @@ public class AMRMProxyService extends CompositeService
implements
private FederationStateStoreFacade federationFacade;
private boolean federationEnabled = false;
+ private final AtomicInteger allocatedCount;
+ private final AtomicInteger askCount;
Review Comment:
Not sure this name is very self explanatory.
Is this the number of requests we got?
requestCount would be more intuitive.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/amrmproxy/AMRMProxyService.java:
##########
@@ -409,7 +420,7 @@ public void
processApplicationStartRequest(StartContainerRequest request)
return;
}
LOG.info("Callback received for initializing request "
Review Comment:
As we are touching, make it single line.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/amrmproxy/AMRMProxyService.java:
##########
@@ -779,7 +792,7 @@ private RequestInterceptorChainWrapper getInterceptorChain(
.getApplicationId())) {
throw new YarnException(
"The AM request processing pipeline is not initialized for app: "
- + appAttemptId.getApplicationId().toString());
+ + appAttemptId.getApplicationId().toString());
Review Comment:
Do we need toString()?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/amrmproxy/AMRMProxyService.java:
##########
@@ -588,74 +592,83 @@ protected void stopApplication(ApplicationId
applicationId) {
this.nmContext.getNMStateStore()
.removeAMRMProxyAppContext(pipeline.getApplicationAttemptId());
} catch (IOException e) {
- LOG.error("Error removing AMRMProxy application context for "
- + applicationId, e);
+ LOG.error("Error removing AMRMProxy application context for {}.",
+ applicationId, e);
+ isStopSuccess = false;
}
}
}
+
+ if(isStopSuccess) {
+ long endTime = clock.getTime();
+ this.metrics.succeededAppStopRequests(endTime - startTime);
+ } else {
+ this.metrics.incrFailedAppStopRequests();
+ }
}
private void updateAMRMTokens(AMRMTokenIdentifier amrmTokenIdentifier,
RequestInterceptorChainWrapper pipeline,
AllocateResponse allocateResponse) {
+
AMRMProxyApplicationContextImpl context =
- (AMRMProxyApplicationContextImpl) pipeline.getRootInterceptor()
- .getApplicationContext();
+ (AMRMProxyApplicationContextImpl)
pipeline.getRootInterceptor().getApplicationContext();
+
+ try {
+ long startTime = clock.getTime();
- // check to see if the RM has issued a new AMRMToken & accordingly update
- // the real ARMRMToken in the current context
- if (allocateResponse.getAMRMToken() != null) {
- LOG.info("RM rolled master-key for amrm-tokens");
+ // check to see if the RM has issued a new AMRMToken & accordingly update
+ // the real ARMRMToken in the current context
+ if (allocateResponse.getAMRMToken() != null) {
+ LOG.info("RM rolled master-key for amrm-tokens.");
- org.apache.hadoop.yarn.api.records.Token token =
- allocateResponse.getAMRMToken();
+ org.apache.hadoop.yarn.api.records.Token token =
+ allocateResponse.getAMRMToken();
- // Do not propagate this info back to AM
- allocateResponse.setAMRMToken(null);
+ // Do not propagate this info back to AM
+ allocateResponse.setAMRMToken(null);
- org.apache.hadoop.security.token.Token<AMRMTokenIdentifier> newToken =
- ConverterUtils.convertFromYarn(token, (Text) null);
+ org.apache.hadoop.security.token.Token<AMRMTokenIdentifier> newToken =
+ ConverterUtils.convertFromYarn(token, (Text) null);
- // Update the AMRMToken in context map, and in NM state store if it is
- // different
- if (context.setAMRMToken(newToken)
- && this.nmContext.getNMStateStore() != null) {
- try {
+ // Update the AMRMToken in context map, and in NM state store if it is
+ // different
+ if (context.setAMRMToken(newToken)
Review Comment:
Single line
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/amrmproxy/AMRMProxyService.java:
##########
@@ -716,8 +729,8 @@ protected RequestInterceptor
createRequestInterceptorChain() {
}
} catch (ClassNotFoundException e) {
throw new YarnRuntimeException(
- "Could not instantiate ApplicationMasterRequestInterceptor: "
- + interceptorClassName, e);
+ "Could not instantiate ApplicationMasterRequestInterceptor: " +
+ interceptorClassName, e);
Review Comment:
This indentation should be different right?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/amrmproxy/AMRMProxyService.java:
##########
@@ -588,74 +592,83 @@ protected void stopApplication(ApplicationId
applicationId) {
this.nmContext.getNMStateStore()
.removeAMRMProxyAppContext(pipeline.getApplicationAttemptId());
} catch (IOException e) {
- LOG.error("Error removing AMRMProxy application context for "
- + applicationId, e);
+ LOG.error("Error removing AMRMProxy application context for {}.",
+ applicationId, e);
+ isStopSuccess = false;
}
}
}
+
+ if(isStopSuccess) {
+ long endTime = clock.getTime();
+ this.metrics.succeededAppStopRequests(endTime - startTime);
+ } else {
+ this.metrics.incrFailedAppStopRequests();
+ }
}
private void updateAMRMTokens(AMRMTokenIdentifier amrmTokenIdentifier,
RequestInterceptorChainWrapper pipeline,
AllocateResponse allocateResponse) {
+
AMRMProxyApplicationContextImpl context =
- (AMRMProxyApplicationContextImpl) pipeline.getRootInterceptor()
- .getApplicationContext();
+ (AMRMProxyApplicationContextImpl)
pipeline.getRootInterceptor().getApplicationContext();
+
+ try {
+ long startTime = clock.getTime();
- // check to see if the RM has issued a new AMRMToken & accordingly update
- // the real ARMRMToken in the current context
- if (allocateResponse.getAMRMToken() != null) {
- LOG.info("RM rolled master-key for amrm-tokens");
+ // check to see if the RM has issued a new AMRMToken & accordingly update
+ // the real ARMRMToken in the current context
+ if (allocateResponse.getAMRMToken() != null) {
+ LOG.info("RM rolled master-key for amrm-tokens.");
- org.apache.hadoop.yarn.api.records.Token token =
- allocateResponse.getAMRMToken();
+ org.apache.hadoop.yarn.api.records.Token token =
+ allocateResponse.getAMRMToken();
- // Do not propagate this info back to AM
- allocateResponse.setAMRMToken(null);
+ // Do not propagate this info back to AM
+ allocateResponse.setAMRMToken(null);
- org.apache.hadoop.security.token.Token<AMRMTokenIdentifier> newToken =
- ConverterUtils.convertFromYarn(token, (Text) null);
+ org.apache.hadoop.security.token.Token<AMRMTokenIdentifier> newToken =
+ ConverterUtils.convertFromYarn(token, (Text) null);
- // Update the AMRMToken in context map, and in NM state store if it is
- // different
- if (context.setAMRMToken(newToken)
- && this.nmContext.getNMStateStore() != null) {
- try {
+ // Update the AMRMToken in context map, and in NM state store if it is
+ // different
+ if (context.setAMRMToken(newToken)
+ && this.nmContext.getNMStateStore() != null) {
this.nmContext.getNMStateStore().storeAMRMProxyAppContextEntry(
context.getApplicationAttemptId(), NMSS_AMRMTOKEN_KEY,
- newToken.encodeToUrlString().getBytes("UTF-8"));
- } catch (IOException e) {
- LOG.error("Error storing AMRMProxy application context entry for "
- + context.getApplicationAttemptId(), e);
+ newToken.encodeToUrlString().getBytes(StandardCharsets.UTF_8));
}
}
- }
- // Check if the local AMRMToken is rolled up and update the context and
- // response accordingly
- MasterKeyData nextMasterKey =
- this.secretManager.getNextMasterKeyData();
-
- if (nextMasterKey != null
- && nextMasterKey.getMasterKey().getKeyId() != amrmTokenIdentifier
- .getKeyId()) {
- Token<AMRMTokenIdentifier> localToken = context.getLocalAMRMToken();
- if (nextMasterKey.getMasterKey().getKeyId() != context
- .getLocalAMRMTokenKeyId()) {
- LOG.info("The local AMRMToken has been rolled-over."
- + " Send new local AMRMToken back to application: "
- + pipeline.getApplicationId());
- localToken =
- this.secretManager.createAndGetAMRMToken(pipeline
- .getApplicationAttemptId());
- context.setLocalAMRMToken(localToken);
- }
+ // Check if the local AMRMToken is rolled up and update the context and
+ // response accordingly
+ MasterKeyData nextMasterKey = this.secretManager.getNextMasterKeyData();
+
+ if (nextMasterKey != null
+ && nextMasterKey.getMasterKey().getKeyId() != amrmTokenIdentifier
+ .getKeyId()) {
+ Token<AMRMTokenIdentifier> localToken = context.getLocalAMRMToken();
+ if (nextMasterKey.getMasterKey().getKeyId() !=
context.getLocalAMRMTokenKeyId()) {
+ LOG.info("The local AMRMToken has been rolled-over."
+ + " Send new local AMRMToken back to application: {}",
+ pipeline.getApplicationId());
+ localToken =
this.secretManager.createAndGetAMRMToken(pipeline.getApplicationAttemptId());
+ context.setLocalAMRMToken(localToken);
+ }
- allocateResponse
- .setAMRMToken(org.apache.hadoop.yarn.api.records.Token
- .newInstance(localToken.getIdentifier(), localToken
- .getKind().toString(), localToken.getPassword(),
- localToken.getService().toString()));
+ allocateResponse
+ .setAMRMToken(org.apache.hadoop.yarn.api.records.Token
+ .newInstance(localToken.getIdentifier(), localToken
Review Comment:
The old indentation looked correct.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/amrmproxy/AMRMProxyService.java:
##########
@@ -418,7 +429,7 @@ public void
processApplicationStartRequest(StartContainerRequest request)
if (amrmToken == null) {
throw new YarnRuntimeException(
"AMRMToken not found in the start container request for "
- + "application:" + appAttemptId.toString());
+ + "application:" + appAttemptId);
Review Comment:
single line?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/amrmproxy/AMRMProxyService.java:
##########
@@ -588,74 +592,83 @@ protected void stopApplication(ApplicationId
applicationId) {
this.nmContext.getNMStateStore()
.removeAMRMProxyAppContext(pipeline.getApplicationAttemptId());
} catch (IOException e) {
- LOG.error("Error removing AMRMProxy application context for "
- + applicationId, e);
+ LOG.error("Error removing AMRMProxy application context for {}.",
+ applicationId, e);
+ isStopSuccess = false;
}
}
}
+
+ if(isStopSuccess) {
+ long endTime = clock.getTime();
+ this.metrics.succeededAppStopRequests(endTime - startTime);
+ } else {
+ this.metrics.incrFailedAppStopRequests();
+ }
}
private void updateAMRMTokens(AMRMTokenIdentifier amrmTokenIdentifier,
RequestInterceptorChainWrapper pipeline,
AllocateResponse allocateResponse) {
+
AMRMProxyApplicationContextImpl context =
- (AMRMProxyApplicationContextImpl) pipeline.getRootInterceptor()
- .getApplicationContext();
+ (AMRMProxyApplicationContextImpl)
pipeline.getRootInterceptor().getApplicationContext();
+
+ try {
+ long startTime = clock.getTime();
- // check to see if the RM has issued a new AMRMToken & accordingly update
- // the real ARMRMToken in the current context
- if (allocateResponse.getAMRMToken() != null) {
- LOG.info("RM rolled master-key for amrm-tokens");
+ // check to see if the RM has issued a new AMRMToken & accordingly update
+ // the real ARMRMToken in the current context
+ if (allocateResponse.getAMRMToken() != null) {
+ LOG.info("RM rolled master-key for amrm-tokens.");
- org.apache.hadoop.yarn.api.records.Token token =
- allocateResponse.getAMRMToken();
+ org.apache.hadoop.yarn.api.records.Token token =
Review Comment:
single line
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/amrmproxy/AMRMProxyService.java:
##########
@@ -867,19 +878,20 @@ public static class RequestInterceptorChainWrapper {
/**
* Initializes the wrapper with the specified parameters.
*
- * @param rootInterceptor the root request intercepter
+ * @param rootInterceptor the root request interceptor
* @param applicationAttemptId attempt id
*/
+ @SuppressWarnings("checkstyle:HiddenField")
Review Comment:
Can we rename the parameter to "rootIntercept" and "appAttemptId"?
--
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]
