[GitHub] [samza] kw2542 commented on a change in pull request #1361: SAMZA-2525:Fix race condition in ClientHelper#isActiveApplication

[GitBox <gi...@apache.org>]

kw2542 commented on a change in pull request #1361:
URL: https://github.com/apache/samza/pull/1361#discussion_r425997244



##########
File path: samza-yarn/src/main/scala/org/apache/samza/job/yarn/ClientHelper.scala
##########
@@ -301,8 +301,8 @@ class ClientHelper(conf: Configuration) extends Logging {
   }
 
   private def isActiveApplication(applicationReport: ApplicationReport): Boolean = {
-    (Running.equals(toAppStatus(applicationReport).get)
-    || New.equals(toAppStatus(applicationReport).get))
+    val status = toAppStatus(applicationReport).get

Review comment:
       You are right applicationReport does not change, however, there is another check in ClientHelper#allContainersRunning to check metrics against AM on "needed-containers"
   
   if needed-container is non zero, the status will be be NEW
   only when needed-containers is zero, the status will be returned as RUNNING.
   
   See
   
   ```
   def allContainersRunning(applicationReport: ApplicationReport): Boolean = {
       val amClient: ApplicationMasterRestClient = createAmClient(applicationReport)
   
       debug("Created client: " + amClient.toString)
   
       try {
         val metrics = amClient.getMetrics
         debug("Got metrics: " + metrics.toString)
   
         val neededContainers = Integer.parseInt(
           metrics.get(classOf[SamzaAppMasterMetrics].getCanonicalName)
             .get("needed-containers")
             .toString)
   
         info("Needed containers: " + neededContainers)
         if (neededContainers == 0) {
           true
         } else {
           false
         }
   ```
   
   and 
   ClientHelper#toAppStatus
   ```      
   case (YarnApplicationState.RUNNING, _) =>
           if (allContainersRunning(applicationReport)) {
             Some(Running)
           } else {
             Some(New)
           }
   ```




----------------------------------------------------------------
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.

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