-
Notifications
You must be signed in to change notification settings - Fork 529
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
Support Mesos fetcher cache (and other options). #637
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,6 +46,10 @@ class Iso8601JobResource @Inject()( | |
"the job's name is invalid. Allowed names: '%s'".format(JobUtils.jobNamePattern.toString())) | ||
if (!Iso8601Expressions.canParse(newJob.schedule, newJob.scheduleTimeZone)) | ||
return Response.status(Response.Status.BAD_REQUEST).build() | ||
if(! JobUtils.isValidURIDefinition(newJob)) { | ||
log.warning(s"Tried to add both uri (deprecated) and fetch parameters on ${newJob.name}") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's Scala string interpolation. I don't think Chronos can be built < Scala 2.10. |
||
return Response.status(Response.Status.BAD_REQUEST).build() | ||
} | ||
|
||
//TODO(FL): Create a wrapper class that handles adding & removing jobs! | ||
jobScheduler.registerJob(List(newJob), persist = true) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,6 +91,7 @@ class JobManagementResource @Inject()(val jobScheduler: JobScheduler, | |
disabled = childJob.disabled, | ||
softError = childJob.softError, | ||
uris = childJob.uris, | ||
fetch = childJob.fetch, | ||
highPriority = childJob.highPriority | ||
) | ||
jobScheduler.updateJob(childJob, newChild) | ||
|
@@ -201,13 +202,19 @@ class JobManagementResource @Inject()(val jobScheduler: JobScheduler, | |
@Timed | ||
def list(): Response = { | ||
try { | ||
val jobs = ListBuffer[BaseJob]() | ||
import scala.collection.JavaConversions._ | ||
jobGraph.dag.vertexSet().map({ | ||
job => | ||
jobs += jobGraph.getJobForName(job).get | ||
}) | ||
Response.ok(jobs.toList).build | ||
val jobs = jobGraph.dag.vertexSet() | ||
.map { jobGraph.getJobForName } | ||
.flatten | ||
.map { // copies fetch in uris or uris in fetch (only one can be set) __only__ in REST get, for compatibility | ||
case j : ScheduleBasedJob => | ||
if(j.fetch.isEmpty) j.copy(fetch = j.uris.map { Fetch(_) }) | ||
else j.copy(uris = j.fetch.map { _.uri }) | ||
case j : DependencyBasedJob => | ||
if(j.fetch.isEmpty) j.copy(fetch = j.uris.map { Fetch(_) }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code is duplicated on matching each type. Consider moving it to a method. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This cannot be moved to a method, due to the fact that there is a case class duplicate between Best course of action would be to refactor to remove duplicates in the model (out of scope of this PR). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Due to the lack of standard copy command this is okay. |
||
else j.copy(uris = j.fetch.map { _.uri }) | ||
} | ||
Response.ok(jobs).build | ||
} catch { | ||
case ex: Exception => | ||
log.log(Level.WARNING, "Exception while serving request", ex) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
package org.apache.mesos.chronos.scheduler.jobs | ||
|
||
import com.fasterxml.jackson.annotation.JsonProperty | ||
|
||
/** | ||
* Created by Sylvain Veyrié on 17/02/2016. | ||
*/ | ||
case class Fetch( | ||
@JsonProperty uri: String, | ||
@JsonProperty executable: Boolean = false, | ||
@JsonProperty cache: Boolean = false, | ||
@JsonProperty extract: Boolean = false) |
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.
In terms of style across the repo there is a space following each 'if' statement but in your commit every single 'if' lacks the space between the opening parentheses. Consider adding the space to keep consistent style and removing the space between the negation (!) and statement.
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 can provide either new commit or amend this commit.