never register org.apache.hadoop.yarn.server.resourcemanager.security.ClientToAMTokenSecretManagerInRM.masterKeys while SecretKey is null.

Hi mujunchao, thanks for reporting and working on this issue.
It is a nice catch. I see why this is a critical issue. For non-secure cluster, the more completed jobs, the more entries with null value will be left in ClientToAMTokenSecretManagerInRM#masterKeys. You patch makes sense to me, since we only call unRegisterApplication in secure mode, we should also call registerApplication in secure mode to match unRegisterApplication.
Could you add a test case in your patch? You can do something similar as TestRMAppAttemptTransitions#testGetClientToken for non-secure mode.

Thanks mujunchao for reporting and working in this. I am assigning this issue to you.

Adding to zhihai xu, Can you also take care of the naming convention for the patch like <JIRA-ID>-<patch-version>.patch?

Nice Catch!!

Thanks for your reviewing, I will give the test case.

Ok, got, thanks Devaraj for reviewing.

add test case.

thanks for the updated patch mujunchao!
The patch looks most good to me, some nits:

1. Add @VisibleForTesting before function hasMasterKey to mark this function used for test only. So you can remove the comment // Only for test
2. It looks like the code in testNoSecureNoRegistClientToken are similar as testRegistClientTokenInSecure. Can we merge testNoSecureNoRegistClientToken with testRegistClientTokenInSecure to one test? We can rename the test as testApplicationAttemptMasterKey. You can check isMasterKeyExisted based on isSecurityEnabled. You can change your comments can not get ClientToken/can get ClientToken to can not get MasterKey/can get MasterKey

update test case, thanks for xuzhihai's reviewing.

thanks for your reviewing, have updated the test case, thanks!

thanks for your reviewing, have updated the test case, thanks!

thanks for updating the patch quickly.
The new patch looks good in general. It'd be nice to address a few small nits, mostly regarding the comment and style.

1. Change // this is to test master key is saved in the state store to // this is to test master key is saved in the secret manager
2. Remove comment //assumeTrue(!isSecurityEnabled);
3. Change if(isSecurityEnabled) to if (isSecurityEnabled)
4. Could you fix the indentation? It should be indented by 2(new paragraph) or 4 bytes.
   +1 non-binding once these are addressed. Thanks again for working on this.

Thanks mujunchao for updated patch with test.

Please take care of these comments also along with the zhihai xu comments fix.

1.I don't think adding this new method is required. Can we just use the ClientToAMTokenSecretManagerInRM#getMasterKey() to know whether the master key present or not?

+  
+  @VisibleForTesting
+  public synchronized boolean hasMasterKey(
+	  ApplicationAttemptId applicationAttemptID) {
+	return this.masterKeys.containsKey(applicationAttemptID);
+  }

2. I see there are some format issues in the patch w.r.t braces and indentation with spaces. Please go through the 'Making Changes' section in https://wiki.apache.org/hadoop/HowToContribute and configure your IDE according. It will be one time job and you don't have to worry next time for creating patches.

+    if(isSecurityEnabled)
+    {

+    }
+    else
+    {

4. Remove unused imports in RMAppAttemptImpl.java.

-1 overall

This message was automatically generated.

Cancelling patch to address the above comments.

fix the comments and format the code.

thanks for your reviewing, have fixed the comments and the indentation.

Thanks Devar for reviewing,
1. I think ClientToAMTokenSecretManagerInRM#hasMasterKey() is necessary, in this case the value is null, so i new function to recognize it.
2. have fixed.
3. As i use annotation @VisibleForTesting,in my ide, need to import com.google.common.annotations.VisibleForTesting.

-1 overall

This message was automatically generated.

remove the whitespace.

remove the whitespace.

+1 overall

This message was automatically generated.

+1 for the latest patch, If no objection, I will commit it tomorrow.

FAILURE: Integrated in Hadoop-trunk-Commit #8317 (See https://builds.apache.org/job/Hadoop-trunk-Commit/8317/)
YARN-3857: Memory leak in ResourceManager with SIMPLE mode. Contributed by mujunchao. (zxu: rev 3a76a010b85176f2bcb85ed6f74c25dcb8acfe4d)

• hadoop-yarn-project/CHANGES.txt
• 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-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/security/ClientToAMTokenSecretManagerInRM.java
• 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

Thanks to mujunchao for the contribution and to Devaraj for additional review! I committed this to trunk, branch-2 and branch-2.7.

FAILURE: Integrated in Hadoop-Yarn-trunk #1022 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/1022/)
YARN-3857: Memory leak in ResourceManager with SIMPLE mode. Contributed by mujunchao. (zxu: rev 3a76a010b85176f2bcb85ed6f74c25dcb8acfe4d)

• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/security/ClientToAMTokenSecretManagerInRM.java
• 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-yarn-project/CHANGES.txt
• 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

FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #292 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/292/)
YARN-3857: Memory leak in ResourceManager with SIMPLE mode. Contributed by mujunchao. (zxu: rev 3a76a010b85176f2bcb85ed6f74c25dcb8acfe4d)

• 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-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/security/ClientToAMTokenSecretManagerInRM.java
• hadoop-yarn-project/CHANGES.txt
• 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

FAILURE: Integrated in Hadoop-Hdfs-trunk #2219 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2219/)
YARN-3857: Memory leak in ResourceManager with SIMPLE mode. Contributed by mujunchao. (zxu: rev 3a76a010b85176f2bcb85ed6f74c25dcb8acfe4d)

• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/security/ClientToAMTokenSecretManagerInRM.java
• 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-yarn-project/CHANGES.txt
• 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

FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #281 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/281/)
YARN-3857: Memory leak in ResourceManager with SIMPLE mode. Contributed by mujunchao. (zxu: rev 3a76a010b85176f2bcb85ed6f74c25dcb8acfe4d)

• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/security/ClientToAMTokenSecretManagerInRM.java
• 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-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-yarn-project/CHANGES.txt

FAILURE: Integrated in Hadoop-Mapreduce-trunk #2238 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2238/)
YARN-3857: Memory leak in ResourceManager with SIMPLE mode. Contributed by mujunchao. (zxu: rev 3a76a010b85176f2bcb85ed6f74c25dcb8acfe4d)

• hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/security/ClientToAMTokenSecretManagerInRM.java
• hadoop-yarn-project/CHANGES.txt
• 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-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/rmapp/attempt/TestRMAppAttemptTransitions.java

FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #289 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/289/)
YARN-3857: Memory leak in ResourceManager with SIMPLE mode. Contributed by mujunchao. (zxu: rev 3a76a010b85176f2bcb85ed6f74c25dcb8acfe4d)

• 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-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-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/security/ClientToAMTokenSecretManagerInRM.java
• hadoop-yarn-project/CHANGES.txt

Does this issue exist in 2.6.x? Should this be backported to branch-2.6?

Yes, this issue exists in 2.6.x, I just committed this patch to branch-2.6.

FAILURE: Integrated in Hadoop-trunk-Commit #8969 (See https://builds.apache.org/job/Hadoop-trunk-Commit/8969/)
Update CHANGES.txt to add YARN-3857 and YARN-3535 to branch-2.6 (zxu: rev 0c3a53e5a978140e56b9ebbc82c8d04fc978e640)

• hadoop-yarn-project/CHANGES.txt

SUCCESS: Integrated in Hadoop-Hdfs-trunk-Java8 #694 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/694/)
Update CHANGES.txt to add YARN-3857 and YARN-3535 to branch-2.6 (zxu: rev 0c3a53e5a978140e56b9ebbc82c8d04fc978e640)

• hadoop-yarn-project/CHANGES.txt