[
https://issues.apache.org/jira/browse/YARN-11289?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17598560#comment-17598560
]
ASF GitHub Bot commented on YARN-11289:
---------------------------------------
goiri commented on code in PR #4836:
URL: https://github.com/apache/hadoop/pull/4836#discussion_r959903262
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/amrmproxy/FederationInterceptor.java:
##########
@@ -770,7 +771,7 @@ public FinishApplicationMasterResponse
finishApplicationMaster(
if (failedToUnRegister) {
homeResponse.setIsUnregistered(false);
- } else {
+ } else if
(request.getFinalApplicationStatus().equals(FinalApplicationStatus.SUCCEEDED)) {
Review Comment:
To prevent null pointers, it is usually to do it the other way.
```
} else if
(FinalApplicationStatus.SUCCEEDED.equals(request.getFinalApplicationStatus()) {
```
At the same time, we need to check request is not null before.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/amrmproxy/FederationInterceptor.java:
##########
@@ -817,6 +818,11 @@ public void shutdown() {
this.homeHeartbeartHandler.shutdown();
this.homeRMRelayer.shutdown();
+ // Shutdown needs to clean up app
+ if (this.registryClient != null) {
+
this.registryClient.removeAppFromRegistry(this.attemptId.getApplicationId());
Review Comment:
Extract appId
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/amrmproxy/TestFederationInterceptor.java:
##########
@@ -1022,4 +1024,125 @@ public void testBatchFinishApplicationMaster() throws
IOException, InterruptedEx
return null;
});
}
+
+ @Test
+ public void testRemoveAppFromRegistryApplicationSuccess()
+ throws IOException, InterruptedException {
+
+ final RegisterApplicationMasterRequest registerReq =
+ Records.newRecord(RegisterApplicationMasterRequest.class);
+ registerReq.setHost(Integer.toString(testAppId));
+ registerReq.setRpcPort(testAppId);
+ registerReq.setTrackingUrl("");
+
+ UserGroupInformation ugi =
interceptor.getUGIWithToken(interceptor.getAttemptId());
+
+ ugi.doAs((PrivilegedExceptionAction<Object>) () -> {
+
+ // Register the application
+ RegisterApplicationMasterRequest registerReq1 =
+ Records.newRecord(RegisterApplicationMasterRequest.class);
+ registerReq1.setHost(Integer.toString(testAppId));
+ registerReq1.setRpcPort(0);
+ registerReq1.setTrackingUrl("");
+
+ // Register ApplicationMaster
+ RegisterApplicationMasterResponse registerResponse =
+ interceptor.registerApplicationMaster(registerReq1);
+ Assert.assertNotNull(registerResponse);
+ lastResponseId = 0;
+
+ Assert.assertEquals(0, interceptor.getUnmanagedAMPoolSize());
+
+ // Allocate the first batch of containers, with sc1 active
+ registerSubCluster(SubClusterId.newInstance("SC-1"));
+
+ int numberOfContainers = 3;
+ List<Container> containers =
+ getContainersAndAssert(numberOfContainers, numberOfContainers);
Review Comment:
indentation
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/amrmproxy/TestFederationInterceptor.java:
##########
@@ -1022,4 +1024,125 @@ public void testBatchFinishApplicationMaster() throws
IOException, InterruptedEx
return null;
});
}
+
+ @Test
+ public void testRemoveAppFromRegistryApplicationSuccess()
+ throws IOException, InterruptedException {
+
+ final RegisterApplicationMasterRequest registerReq =
+ Records.newRecord(RegisterApplicationMasterRequest.class);
+ registerReq.setHost(Integer.toString(testAppId));
+ registerReq.setRpcPort(testAppId);
+ registerReq.setTrackingUrl("");
+
+ UserGroupInformation ugi =
interceptor.getUGIWithToken(interceptor.getAttemptId());
+
+ ugi.doAs((PrivilegedExceptionAction<Object>) () -> {
+
+ // Register the application
+ RegisterApplicationMasterRequest registerReq1 =
+ Records.newRecord(RegisterApplicationMasterRequest.class);
+ registerReq1.setHost(Integer.toString(testAppId));
+ registerReq1.setRpcPort(0);
+ registerReq1.setTrackingUrl("");
+
+ // Register ApplicationMaster
+ RegisterApplicationMasterResponse registerResponse =
+ interceptor.registerApplicationMaster(registerReq1);
Review Comment:
indentation
> [Federation] Improve NM FederationInterceptor removeAppFromRegistry
> -------------------------------------------------------------------
>
> Key: YARN-11289
> URL: https://issues.apache.org/jira/browse/YARN-11289
> Project: Hadoop YARN
> Issue Type: Improvement
> Components: federation, nodemanager
> Affects Versions: 3.4.0
> Reporter: fanshilun
> Assignee: fanshilun
> Priority: Major
> Labels: pull-request-available
>
> [Federation] Improve NM FederationInterceptor removeAppFromRegistry
> 1.FederationInterceptor#finishApplicationMaster needs to check
> getFinalApplicationStatus, and clean up when the status is SUCCESS.
> 2. When FederationInterceptor#shutdown, perform CleanUp on Application.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
