-
Notifications
You must be signed in to change notification settings - Fork 9.1k
YARN-9013. [BackPort] [GPG] fix order of steps cleaning Registry entries in ApplicationCleaner. #6147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ies in ApplicationCleaner.
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
import org.apache.hadoop.yarn.server.federation.store.records.ApplicationHomeSubCluster; | ||
import org.apache.hadoop.yarn.server.federation.store.records.GetApplicationsHomeSubClusterRequest; | ||
import org.apache.hadoop.yarn.server.federation.store.records.SubClusterId; | ||
import org.apache.hadoop.yarn.server.federation.store.records.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@@ -4367,7 +4367,7 @@ public static boolean isAclEnabled(Configuration conf) { | |||
* the default value is 0s. | |||
*/ | |||
public static final long DEFAULT_ROUTER_USER_CLIENT_THREAD_POOL_KEEP_ALIVE_TIME = | |||
TimeUnit.SECONDS.toMillis(0); // 0s | |||
TimeUnit.SECONDS.toMillis(30); // 0s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment needs to be updated.
Also not sure what's the agreement on changing default values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for discovering this issue! It was my oversight, and I introduced this problem while conducting integration testing. I will fix it.
GetApplicationsHomeSubClusterResponse applicationsHomeSubCluster = | ||
stateStore.getApplicationsHomeSubCluster(appHomeSubClusterRequest); | ||
int size = applicationsHomeSubCluster.getAppsHomeSubClusters().size(); | ||
Assert.assertEquals(1, size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, either extract both or none.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will improve it.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@goiri Thank you very much for your help in reviewing the code! |
…ies in ApplicationCleaner. (apache#6147) Contributed by Botong Huang, Shilun Fan. Co-authored-by: Botong Huang <botong@apache.org> Reviewed-by: Inigo Goiri <inigoiri@apache.org> Signed-off-by: Shilun Fan <slfan1989@apache.org>
Description of PR
JIRA: YARN-9013. [BackPort] [GPG] fix order of steps cleaning Registry entries in ApplicationCleaner.
How was this patch tested?
Add Junit Test.
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?