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

Support Mesos fetcher cache (and other options). #637

Merged
merged 3 commits into from
Feb 29, 2016

Conversation

turb
Copy link
Contributor

@turb turb commented Feb 19, 2016

Implements #611.

The replaces the uri option by a fetch structure that follows Mesos Fetcher configuration options.

Those options are:

  • uri
  • cache (each mesos slave involved will cache the file on disk)
  • extract (the file is an archive that will be extracted)
  • executable (the file will be chown to be executable)

This has two consequences:

  • it deprecates the uri option (for now, is mapped to fetch with all boolean options to false).
  • it requires Mesos 0.23, so Chronos now requires it.

* Deprecates "uri" option.
* Requires Mesos 0.23
@turb
Copy link
Contributor Author

turb commented Feb 19, 2016

Note about Travis check: testElectLeaderOnZkFailure seems to be linked to network (latency?), and passes or fails randomly. AFAIK it has nothing to do with this PR.

@philipnrmn philipnrmn self-assigned this Feb 24, 2016
@philipnrmn
Copy link
Contributor

Hi @turb,

Thanks for the pull request! It looks pretty good, however there are a couple of issue to do with the way the API functions.

  • The /scheduler/iso8601 endpoint currently accepts both uris and fetch fields in job configurations.
  • The /jobs endpoint only returns those fields which were posted.

May I suggest we use the same rules as Marathon to deal with these two fields? We should allow the user to specify either uris or fetch but not both, and we should return both from the /jobs endpoint.

@turb
Copy link
Contributor Author

turb commented Feb 25, 2016

@philipnrmn thanks for the feedback!

On those two points, two questions:

  • I agree on the restriction leading the user to migrate to the new parameter. if I am not mistaken, I should add a constraint in the block starting at JobDeserializer:199. Am I right?
  • To always return both parameters, I don't find any case in JobSerializer that distinguishes between persistence serialization and API endpoint serialization. Did I miss something? If this distinction is not made, it means we would persist both, hence duplicates in configuration that would be boggy to manage (maybe by excluding duplicates in JobUtils.fromBytes).

@turb
Copy link
Contributor Author

turb commented Feb 25, 2016

It seems I have been mistaken on constraints, so let's restart the first point.

Bean validation is initialized in ChronosRestModule with Mesosphere's JacksonMessageBodyProvider, but, as far as I see, used nowhere in Chronos. I will then add validation in Iso8601JobResource.

@turb
Copy link
Contributor Author

turb commented Feb 25, 2016

@philipnrmn OK, nevermind, I changed the code to implement both points and now have to test it.

Do you prefer another commit, or an amendment to the current one?

@philipnrmn
Copy link
Contributor

Wow you respond quicker than I can! I was halfway through replying when your latest comment appeared.

I'd prefer a separate commit, if that's OK with you.

@turb
Copy link
Contributor Author

turb commented Feb 25, 2016

@philipnrmn that's because I decided to really enter in Chronos codebase, to understand how things are done, and my messages reflect this process :)

I am afraid I have nearly rewritten JobManagementResource.list to find back the usual Scala semantics I am used to. It will change one behaviour: if jobGraph.getJobForName returns None, the element will be ignored, instead of throwing an exception.

The change will only be on the REST JSON for compatibility (and avoid the problems I mentioned in a previous message).

I will be able to test this on our Mesos cluster only once it is available for disturbance, so if everything goes well the commit (with both modifications) will be pushed by the end of the week.

@philipnrmn
Copy link
Contributor

Amazing! Thanks for this contribution!

@turb
Copy link
Contributor Author

turb commented Feb 26, 2016

Now available for review.

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(_) })
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 ScheduleBasedJob and DependencyBasedJob. An alternative would be to add one case class standard copy() in the trait, just for this one usage; I don't like too much the idea.

Best course of action would be to refactor to remove duplicates in the model (out of scope of this PR).

Copy link
Contributor

Choose a reason for hiding this comment

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

Due to the lack of standard copy command this is okay.

@turb
Copy link
Contributor Author

turb commented Feb 29, 2016

@philipnrmn if think this PR is ready to merge ;)

@philipnrmn
Copy link
Contributor

Thanks for the additional eyes, @Califax, and thanks very much @turb for the PR!

philipnrmn added a commit that referenced this pull request Feb 29, 2016
Support Mesos fetcher cache (and other options).
@philipnrmn philipnrmn merged commit 8fdb3d8 into mesos:master Feb 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants