You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@samza.apache.org by GitBox <gi...@apache.org> on 2020/05/15 18:47:52 UTC
[GitHub] [samza] kw2542 opened a new pull request #1361: SAMZA-2525:Fix race condition in ClientHelper#isActiveApplication
kw2542 opened a new pull request #1361:
URL: https://github.com/apache/samza/pull/1361
Symptom:Incorrect job status reported, which is previous deployed job status instead of current job.
Cause:ClientHelper#isActiveApplication is invoking toAppStatus twice when comparing status to Running or New. It could happen that 1st time, the status is New which failed Running check, and 2nd invocation returns Running, which failed New check, this will result in the current app to be classified as non active causing ClientHelper to return job status of a previous deployed app.
Changes: Update ClientHelper#isActiveApplication to invoke toAppStatus once and compare both status against this single result.
Tests: None
API Changes: None
Upgrade Instructions: None
Usage Instructions: None
----------------------------------------------------------------
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
[GitHub] [samza] kw2542 commented on a change in pull request #1361: SAMZA-2525:Fix race condition in ClientHelper#isActiveApplication
Posted by 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
[GitHub] [samza] lakshmi-manasa-g commented on a change in pull request #1361: SAMZA-2525:Fix race condition in ClientHelper#isActiveApplication
Posted by GitBox <gi...@apache.org>.
lakshmi-manasa-g commented on a change in pull request #1361:
URL: https://github.com/apache/samza/pull/1361#discussion_r425998328
##########
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:
thanks for clarifying. makes sense
----------------------------------------------------------------
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
[GitHub] [samza] lakshmi-manasa-g commented on a change in pull request #1361: SAMZA-2525:Fix race condition in ClientHelper#isActiveApplication
Posted by GitBox <gi...@apache.org>.
lakshmi-manasa-g commented on a change in pull request #1361:
URL: https://github.com/apache/samza/pull/1361#discussion_r425994565
##########
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:
but application report does not change.. so how would toAppStatus return different values for the same applicationReport? i thought applicationReport does NOT change within the same invocation of isActiveAppliction.
----------------------------------------------------------------
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
[GitHub] [samza] cameronlee314 merged pull request #1361: SAMZA-2525:Fix race condition in ClientHelper#isActiveApplication
Posted by GitBox <gi...@apache.org>.
cameronlee314 merged pull request #1361:
URL: https://github.com/apache/samza/pull/1361
----------------------------------------------------------------
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