Skip to content
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

SAMZA-1250: JobRunner.kill doesn't terminate cleanly with YarnJob. #152

Closed
wants to merge 5 commits into from

Conversation

jmakes
Copy link
Contributor

@jmakes jmakes commented May 1, 2017

  1. The ClientHelper now checks inactive application IDs so it can get status for terminated jobs in addition to running jobs
  2. JobRunner.kill() waits for any finish, not just successful finish.
  3. A killed job is now considered successful.

Copy link
Contributor

@prateekm prateekm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Minor readability suggestions.


getAppsRsp
.asScala
.filter(appRep => ( !(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Maybe be cleaner to write this condition as a multi-line block using {} with an extracted val for appStatus.
Also, not clear what Rsp and Rep are. Maybe use a more verbose name?

@@ -249,8 +249,23 @@ class ClientHelper(conf: Configuration) extends Logging {
.toList
}

def getPreviousApplicationIds(appName: String): List[ApplicationId] = {
val getAppsRsp = yarnClient.getApplications
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: val apps or whatever that type is?

@@ -279,7 +294,7 @@ class ClientHelper(conf: Configuration) extends Logging {

private def convertState(state: YarnApplicationState, status: FinalApplicationStatus): Option[ApplicationStatus] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: toAppStatus?

applicationIds.sorted.reverse.headOption
} else {
// Couldn't find an active applicationID. Use one the latest finished ID.
val pastApplicationIds = client.getPreviousApplicationIds(applicationName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Do we want to log this?

@asfgit asfgit closed this in a1e03af May 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants