Details
-
Bug
-
Status: Closed
-
Major
-
Resolution: Fixed
-
None
-
None
-
None
-
Reviewed
Description
The jira is tracking why appToken and clientToAMToken is removed separately, and why they are distributed in different transitions, ideally there may be a common place where these two tokens can be removed at the same time.
Attachments
Attachments
- YARN-643.1.patch
- 9 kB
- Xuan Gong
- YARN-643.2.patch
- 10 kB
- Xuan Gong
- YARN-643.3.patch
- 12 kB
- Xuan Gong
- YARN-643.4.patch
- 14 kB
- Xuan Gong
- YARN-643.5.patch
- 14 kB
- Xuan Gong
Activity
I think the common place will be BaseFinalTransition. Because all the transitions which will let AppAttempt move to terminate state(Failed, killed or Finished) will call BaseFinalTransition. So, I move clientToken into BaseFinalTransition, and remove appToken from AMUnregisteredTransition. Also add TokenRemove to AppRejectedTransition which will change appAttempt from submitted to failed
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12595073/YARN-643.1.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 1 new or modified test files.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
-1 core tests. The patch failed these unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager:
org.apache.hadoop.yarn.server.resourcemanager.security.TestAMRMTokens
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-YARN-Build/1617//testReport/
Console output: https://builds.apache.org/job/PreCommit-YARN-Build/1617//console
This message is automatically generated.
+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12595205/YARN-643.2.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 1 new or modified test files.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager.
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-YARN-Build/1626//testReport/
Console output: https://builds.apache.org/job/PreCommit-YARN-Build/1626//console
This message is automatically generated.
Few comments on the patch:
- The tokens are first removed inside AMUnregisteredTransition, it will once again be removed in FinalTransition.
- can you create a common function for removing these tokens instead of replicating the code ?
+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12595500/YARN-643.3.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 2 new or modified test files.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager.
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-YARN-Build/1642//testReport/
Console output: https://builds.apache.org/job/PreCommit-YARN-Build/1642//console
This message is automatically generated.
Patch mostly good,
In the test case, can you also verify AMRMToken is removed ?
why we need to explicitly send the EXPIRE event, doesn't it call finishApplicationMaster already ?
rm.getRMContext()
.getDispatcher()
.getEventHandler()
.handle(
new RMAppAttemptEvent(applicationAttemptId,
RMAppAttemptEventType.EXPIRE));
why we need to explicitly send the EXPIRE event, doesn't it call finishApplicationMaster already ?
This finishApplicationMaster only changed the state to finishing. We did not call tokenRemove at that time.
In the test case, can you also verify AMRMToken is removed ?
We have already called this in the test case:
private void verifyTokenRemoved(ApplicationAttemptId appAttemptId) { verify(amRMTokenManager).applicationMasterFinished(appAttemptId); if (UserGroupInformation.isSecurityEnabled()) { verify(clientToAMTokenManager).unRegisterApplication(appAttemptId); } }
Patch mostly looks good. I like how all the cases are covered.
In TestAMRMTokens, instead of sending an expire event, send RMAppAttemptEventType.CONTAINER_FINISHED. And add a comment why we are sending it explicitly.
Also add an explicit check in TestRMAppAttemptTransitions.testAppAttemptFinishingState that tokens are NOT removed in finishing state.
-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12597636/YARN-643.4.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 2 new or modified test files.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
-1 javadoc. The javadoc tool appears to have generated 2 warning messages.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager.
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-YARN-Build/1701//testReport/
Console output: https://builds.apache.org/job/PreCommit-YARN-Build/1701//console
This message is automatically generated.
[WARNING] org/apache/directory/api/ldap/model/name/Dn.class(org/apache/directory/api/ldap/model/name:Dn.class): warning: Cannot find annotation method 'value()' in type 'edu.umd.cs.findbugs.annotations.SuppressWarnings': class file for edu.umd.cs.findbugs.annotations.SuppressWarnings not found
[WARNING] org/apache/directory/api/ldap/model/name/Dn.class(org/apache/directory/api/ldap/model/name:Dn.class): warning: Cannot find annotation method 'justification()' in type 'edu.umd.cs.findbugs.annotations.SuppressWarnings'
Do not think there two javadoc warnings are related to this patch
You could have just added a new method for validating that tokens are absent. Either that or rename the current method to be verifyTokenCount().
+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12598586/YARN-643.5.patch
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 2 new or modified test files.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 eclipse:eclipse. The patch built with eclipse:eclipse.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed unit tests in hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager.
+1 contrib tests. The patch passed contrib unit tests.
Test results: https://builds.apache.org/job/PreCommit-YARN-Build/1737//testReport/
Console output: https://builds.apache.org/job/PreCommit-YARN-Build/1737//console
This message is automatically generated.
Committed this to trunk, branch-2 and branch-2.1. Thanks Xuan!
Tx for the early reviews and the bug-report, Jian!
SUCCESS: Integrated in Hadoop-trunk-Commit #4289 (See https://builds.apache.org/job/Hadoop-trunk-Commit/4289/)
YARN-643. Fixed ResourceManager to remove all tokens consistently on app finish. Contributed by Xuan Gong. (vinodkv: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1515256)
- /hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt
- /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/rmapp/attempt/RMAppAttemptImpl.java
- /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/rmapp/attempt/TestRMAppAttemptTransitions.java
- /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/security/TestAMRMTokens.java
SUCCESS: Integrated in Hadoop-Yarn-trunk #306 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/306/)
YARN-643. Fixed ResourceManager to remove all tokens consistently on app finish. Contributed by Xuan Gong. (vinodkv: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1515256)
- /hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt
- /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/rmapp/attempt/RMAppAttemptImpl.java
- /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/rmapp/attempt/TestRMAppAttemptTransitions.java
- /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/security/TestAMRMTokens.java
FAILURE: Integrated in Hadoop-Hdfs-trunk #1496 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1496/)
YARN-643. Fixed ResourceManager to remove all tokens consistently on app finish. Contributed by Xuan Gong. (vinodkv: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1515256)
- /hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt
- /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/rmapp/attempt/RMAppAttemptImpl.java
- /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/rmapp/attempt/TestRMAppAttemptTransitions.java
- /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/security/TestAMRMTokens.java
FAILURE: Integrated in Hadoop-Mapreduce-trunk #1523 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1523/)
YARN-643. Fixed ResourceManager to remove all tokens consistently on app finish. Contributed by Xuan Gong. (vinodkv: http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1515256)
- /hadoop/common/trunk/hadoop-yarn-project/CHANGES.txt
- /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/rmapp/attempt/RMAppAttemptImpl.java
- /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/rmapp/attempt/TestRMAppAttemptTransitions.java
- /hadoop/common/trunk/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/security/TestAMRMTokens.java
I did not see any issues if we did not remove clientToAMToken in BaseFinalTransition.
There are three AttemptState after the BaseFinalTransition.
a. RMAppAttemptState.Killed. Not only the appAttempt, but also App will be killed.
b. RMAppAttemptState.FAILED. The RMApp will create a new RMAppAttemp, in which the clientToAMToken will be recreated.
c. RMAppAttemptState.FINISHED. Both RMApp and RMAppAttempt will go to FINISHED state which is the final state.
I think we can have clientToAMToken to be removed in BaseFinalTransition, but it will make no difference.