[GitHub] [hadoop] goiri commented on a diff in pull request #6083: YARN-8862. [BackPort] [GPG] Add Yarn Registry cleanup in ApplicationCleaner.

2023-09-26 Thread via GitHub 


goiri commented on code in PR #6083:
URL: https://github.com/apache/hadoop/pull/6083#discussion_r1337823389


##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-globalpolicygenerator/src/main/java/org/apache/hadoop/yarn/server/globalpolicygenerator/GlobalPolicyGenerator.java:
##
@@ -214,6 +230,10 @@ protected void serviceStart() throws Exception {
 
   @Override
   protected void serviceStop() throws Exception {
+if (this.registry != null) {
+  this.registry.stop();

Review Comment:
   Set to null?



-- 
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: common-issues-unsubscr...@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org

[GitHub] [hadoop] goiri commented on a diff in pull request #6083: YARN-8862. [BackPort] [GPG] Add Yarn Registry cleanup in ApplicationCleaner.

2023-09-25 Thread via GitHub 


goiri commented on code in PR #6083:
URL: https://github.com/apache/hadoop/pull/6083#discussion_r1336380046


##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/webapp/FederationInterceptorREST.java:
##
@@ -761,7 +761,7 @@ public AppsInfo getApps(HttpServletRequest hsr, String 
stateQuery,
 });
 
 if (apps.getApps().isEmpty()) {
-  return null;
+  return apps;

Review Comment:
   Is this what we want?



##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/test/java/org/apache/hadoop/yarn/server/router/webapp/TestRouterWebServicesREST.java:
##
@@ -373,6 +373,8 @@ public void testClusterInfoXML() throws Exception {
 assertEquals(
 rmResponse.getRMVersion(),
 routerResponse.getRMVersion());
+
+Thread.sleep(1000);

Review Comment:
   Debugging?



##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/test/resources/yarn-site.xml:
##
@@ -39,7 +39,7 @@
 yarn.resourcemanager.resource-profiles.source-file
 profiles/sample-profiles-1.json
   
-  
+ 

Review Comment:
   Avoid



-- 
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: common-issues-unsubscr...@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org

[GitHub] [hadoop] goiri commented on a diff in pull request #6083: YARN-8862. [BackPort] [GPG] Add Yarn Registry cleanup in ApplicationCleaner.

2023-09-20 Thread via GitHub 


goiri commented on code in PR #6083:
URL: https://github.com/apache/hadoop/pull/6083#discussion_r1332300958


##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-globalpolicygenerator/src/test/java/org/apache/hadoop/yarn/server/globalpolicygenerator/applicationcleaner/TestDefaultApplicationCleaner.java:
##
@@ -96,6 +119,13 @@ public void breakDown() {
   stateStore.close();
   stateStore = null;
 }
+if (registryClient != null) {
+  registryClient.cleanAllApplications();

Review Comment:
   Can we null the registryClient too?



##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/test/java/org/apache/hadoop/yarn/server/router/subcluster/fair/TestYarnFederationWithFairScheduler.java:
##
@@ -64,6 +64,7 @@ public void testGetClusterInfo() throws InterruptedException, 
IOException {
 FederationClusterInfo federationClusterInfo =
 TestFederationSubCluster.performGetCalls(ROUTER_WEB_ADDRESS, 
RM_WEB_SERVICE_PATH,
 FederationClusterInfo.class, null, null);
+Thread.sleep(200);

Review Comment:
   Debugging?



-- 
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: common-issues-unsubscr...@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org

[GitHub] [hadoop] goiri commented on a diff in pull request #6083: YARN-8862. [BackPort] [GPG] Add Yarn Registry cleanup in ApplicationCleaner.

2023-09-18 Thread via GitHub 


goiri commented on code in PR #6083:
URL: https://github.com/apache/hadoop/pull/6083#discussion_r1328986100


##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/utils/FederationRegistryClient.java:
##
@@ -203,21 +203,27 @@ public synchronized boolean 
writeAMRMTokenForUAM(ApplicationId appId,
* Remove an application from registry.
*
* @param appId application id
+   * @param ignoreMemoryState whether to ignore the memory data in terms of
+   *  known application
*/
-  public synchronized void removeAppFromRegistry(ApplicationId appId) {
+  public synchronized void removeAppFromRegistry(ApplicationId appId,
+  boolean ignoreMemoryState) {
 Map> subClusterTokenMap =
 this.appSubClusterTokenMap.get(appId);
-LOG.info("Removing all registry entries for {}", appId);
-
-if (subClusterTokenMap == null || subClusterTokenMap.size() == 0) {
-  return;
+if (!ignoreMemoryState) {
+  if (subClusterTokenMap == null || subClusterTokenMap.size() == 0) {

Review Comment:
   isEmpty?



##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-globalpolicygenerator/src/main/java/org/apache/hadoop/yarn/server/globalpolicygenerator/applicationcleaner/ApplicationCleaner.java:
##
@@ -148,6 +153,18 @@ public Set getRouterKnownApplications() 
throws YarnException {
 + " success Router queries after " + totalAttemptCount + " retries");
   }
 
+  protected void cleanupAppRecordInRegistry(Set knownApps) {
+List allApps = this.registryClient.getAllApplications();
+LOG.info("Got " + allApps.size() + " existing apps in registry");

Review Comment:
   {} format



##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-globalpolicygenerator/src/test/java/org/apache/hadoop/yarn/server/globalpolicygenerator/applicationcleaner/TestDefaultApplicationCleaner.java:
##
@@ -96,6 +119,12 @@ public void breakDown() {
   stateStore.close();
   stateStore = null;
 }
+if (registryClient != null) {
+  registryClient.cleanAllApplications();
+}
+if (registry != null) {
+  registry.stop();

Review Comment:
   Reset to null?



##
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-common/src/main/java/org/apache/hadoop/yarn/server/federation/utils/FederationRegistryClient.java:
##
@@ -203,21 +203,27 @@ public synchronized boolean 
writeAMRMTokenForUAM(ApplicationId appId,
* Remove an application from registry.
*
* @param appId application id
+   * @param ignoreMemoryState whether to ignore the memory data in terms of
+   *  known application
*/
-  public synchronized void removeAppFromRegistry(ApplicationId appId) {
+  public synchronized void removeAppFromRegistry(ApplicationId appId,
+  boolean ignoreMemoryState) {

Review Comment:
   Can we have a default method with this as false (or true, whatever is the 
common case), to prevent changing the signature?



-- 
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: common-issues-unsubscr...@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org