Re: Review Request 49048: AURORA-1710 Make 'tier' required and remove support for 'production' flag in Job configuration - CLI changes

2016-07-23 Thread Stephan Erb 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49048/#review143319
---


Ship it!




LGTM!

Minor missing things:

* deprecation notice for the user-facing configuration option
* rebase


Joshua, would be great if you could land this patch once its done. I am gone 
for the week & without proper internet access.

- Stephan Erb


On July 20, 2016, 7:56 p.m., Mehrdad Nurolahzade wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49048/
> ---
> 
> (Updated July 20, 2016, 7:56 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Stephan Erb.
> 
> 
> Bugs: AURORA-1710
> https://issues.apache.org/jira/browse/AURORA-1710
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> AURORA-1710 Make 'tier' required and remove support for 'production' flag in 
> Job configuration - CLI changes
> 
> 
> Diffs
> -
> 
>   RELEASE-NOTES.md 29d224d5596eb060356f1343cf347db79b1ad687 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 68baf8fdb90cd26100159401c46c9963c24332b3 
>   src/main/python/apache/aurora/client/cli/context.py 
> 9b1511801d031ff48b81c25688a55cb586b8ac66 
>   src/main/python/apache/aurora/client/config.py 
> 2fc12559016d406c347adb416a5166cca31c961e 
>   src/test/python/apache/aurora/client/cli/test_command_hooks.py 
> 2130f1fa71be02a004cdf8e476a270c81a7105d3 
>   src/test/python/apache/aurora/client/cli/test_context.py 
> 204ca092adad8bf43c5032a02f61bf303fb0b2fc 
>   src/test/python/apache/aurora/client/cli/test_create.py 
> 8c27e2b340bb0a5fb5bcb44ef94d433e7f92c76c 
>   src/test/python/apache/aurora/client/cli/test_cron.py 
> f3c522ed94a2d774865811ceb546bf9df083c14f 
>   src/test/python/apache/aurora/client/cli/test_plugins.py 
> a545fece5e2b3e0017a61e1be9ac478372b1f34d 
>   src/test/python/apache/aurora/client/cli/test_restart.py 
> 967d560e5c7eb0ed85b215fb11d9751b8666acb5 
>   src/test/python/apache/aurora/client/cli/util.py 
> 7b4558ec7f0fb0fd2902591bc6a90dc15051dd6e 
>   src/test/python/apache/aurora/client/test_config.py 
> b1a3c1865819899ef19173be0f861783a2631d0a 
> 
> Diff: https://reviews.apache.org/r/49048/diff/
> 
> 
> Testing
> ---
> 
> ```
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> *** OK (All tests passed) ***
> 
> mesos-master start/running, process 26868
> + RETCODE=0
> + restore_netrc
> + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc
> + true
> Connection to 127.0.0.1 closed.
> 
> real  19m46.324s
> user  0m1.496s
> sys   0m0.774s
> ```
> 
> 
> Thanks,
> 
> Mehrdad Nurolahzade
> 
>

Re: Review Request 49048: AURORA-1710 Make 'tier' required and remove support for 'production' flag in Job configuration - CLI changes

2016-07-23 Thread Stephan Erb 


> On July 20, 2016, 12:27 a.m., Stephan Erb wrote:
> > RELEASE-NOTES.md, line 13
> > 
> >
> > Maybe add here explicitly that `production` is deprecated and that 
> > people should use `tier='preferred'` instead.
> > 
> > Some people are only skimming the deprecation sections when updating, 
> > so they might miss it otherwise.
> 
> Mehrdad Nurolahzade wrote:
> This was added to release notes under `Deprecations and removals` section 
> for 0.14.0 release when scheduler-side backfill work was done:
> ```
> - Deprecated `production` field in `TaskConfig` thrift struct. Use `tier` 
> field to specify task
>   scheduling and resource handling behavior.
> ```
> Would you still suggest we add a deprecation entry to 0.16.0 release note?

Yes, I believe this makes sense to add this deprecation warning again. We are 
now also deprecating the user-facing flag rather than only the one used in the 
Thrift API.


> On July 20, 2016, 12:27 a.m., Stephan Erb wrote:
> > src/main/python/apache/aurora/client/cli/context.py, line 143
> > 
> >
> > Side stepping the `get_config` factory and re-creating the 
> > `AnnotatedAuroraConfig` does not seem optimal in my eyes.
> > 
> > Have you considered using Aurora's binding helper concept instead? Idea 
> > would be: 
> > 
> > * change the default of tier from `None` to `{{aurora.default_tier}}` 
> > (or something similar)
> > * register a binding helper 
> > https://github.com/apache/aurora/blob/528198ecbf4adde22988f6073b043e3da049486d/src/main/python/apache/aurora/client/binding_helper.py#L33
> >  
> > * let the helper automatically resolve the  binding via a call to the 
> > scheduler whenever the user has not set a custom tier
> > 
> > Backfilling of the production flag could be moved to the scheduler side.
> 
> Mehrdad Nurolahzade wrote:
> Thanks for pointing out binding helpers, did not know about them.
> 
> The backfilling of production/tier attributes is already happening on the 
> scheduler side. We did that as part of the same ticket and it has already 
> been merged to master, see [rb 48559](https://reviews.apache.org/r/48559). 
> 
> To idea here is to have the backfill logic repeated on the client side so 
> that if the new client is used against the old scheduler it would still 
> correctly backfill tier and production settings. Perhaps we can throw away 
> this client side backfilling logic in the future and allow it to be done 
> entirely on the scheduler side.

Ok. Let's keep it that way then for now. Maybe we can refactor it in the future 
when the production flag has been removed completely.


> On July 20, 2016, 12:27 a.m., Stephan Erb wrote:
> > src/main/python/apache/aurora/client/config.py, line 122
> > 
> >
> > That comment reminds me we should have solid user facing docs. This 
> > includes:
> > 
> > * a note that `production` is deprecated in favor of tier in 
> > `reference/configuration.md`
> > * according update of `features/multitenancy.md#preemption`
> > * a user facing description of the job tier. Most people have never 
> > heard of it, so we need to explain it to them. Probably a new subsection in 
> > `features/multitenancy.md` is a good place to do this.
> 
> Mehrdad Nurolahzade wrote:
> We can probably track documentation changes under 
> [AURORA-1656](https://issues.apache.org/jira/browse/AURORA-1656), allowing 
> this change-set to be limited to code changes only.

Alright then :). But lets make sure we get that done before the next release


> On July 20, 2016, 12:27 a.m., Stephan Erb wrote:
> > src/test/python/apache/aurora/client/cli/test_cron.py, line 111
> > 
> >
> > As an example of many similar test changes:
> > 
> > The additional mock calls are obscuring the original test intend. A 
> > simple workaround would be to already set a `tier` in the job. 
> > `get_tier_config` would then no longer need to be called.
> 
> Mehrdad Nurolahzade wrote:
> Your suggested workaround does not work. 
> 
> If you read through `_get_config_with_production_and_tier()` you would 
> notice that the call on `api.get_tier_configs()` is made whether or not 
> `tier` is already set. The idea is even when `tier` is set, we need to ensure 
> that the selection of `tier` matches that of `production` flag. For example, 
> `tier=preemptible` and `production=true` should result in `production` being 
> revised to `false`.

Ok.


- Stephan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49048/#review142828
---