aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stephan Erb <s...@apache.org>
Subject Re: Review Request 49048: AURORA-1710 Make 'tier' required and remove support for 'production' flag in Job configuration - CLI changes
Date Sat, 23 Jul 2016 17:10:07 GMT


> On July 20, 2016, 12:27 a.m., Stephan Erb wrote:
> > RELEASE-NOTES.md, line 13
> > <https://reviews.apache.org/r/49048/diff/7/?file=1440195#file1440195line13>
> >
> >     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
> > <https://reviews.apache.org/r/49048/diff/7/?file=1440197#file1440197line143>
> >
> >     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
> > <https://reviews.apache.org/r/49048/diff/7/?file=1440198#file1440198line122>
> >
> >     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
> > <https://reviews.apache.org/r/49048/diff/7/?file=1440202#file1440202line111>
> >
> >     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
-----------------------------------------------------------


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message