You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Neil Conway <ne...@gmail.com> on 2017/04/20 18:18:25 UTC
Re: Review Request 58584: Disabled support for setting quota on
nested roles.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58584/
-----------------------------------------------------------
(Updated April 20, 2017, 6:18 p.m.)
Review request for mesos, Benjamin Mahler, Jay Guo, and Michael Park.
Changes
-------
Tweak description.
Summary (updated)
-----------------
Disabled support for setting quota on nested roles.
Repository: mesos
Description (updated)
-------
Correct support for quota on nested roles will require further work in
the allocator (see MESOS-7402 for details). For now, setting quota on
nested roles is disabled until MESOS-7402 can be fixed. This commit
disables any tests that rely on setting quota on nested roles; it also
adds a (disabled) test to cover the behavior that will be fixed as part
of MESOS-7402.
Diffs (updated)
-----
src/master/quota_handler.cpp 7ff43a048e17b9e9ac0ceed248f7b3fd56b007d6
src/tests/hierarchical_allocator_tests.cpp 33e7b455f8664858eb4f03727b076a10c80cd6e0
src/tests/master_quota_tests.cpp 1714ba13ea63bae05448d0898bf722ef472c672b
Diff: https://reviews.apache.org/r/58584/diff/2/
Changes: https://reviews.apache.org/r/58584/diff/1-2/
Testing
-------
`make check`
Thanks,
Neil Conway
Re: Review Request 58584: Disabled support for setting quota on
nested roles.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58584/
-----------------------------------------------------------
(Updated April 24, 2017, 3:04 p.m.)
Review request for mesos, Benjamin Mahler, Jay Guo, and Michael Park.
Changes
-------
Address review comments.
Repository: mesos
Description
-------
Correct support for quota on nested roles will require further work in
the allocator (see MESOS-7402 for details). For now, setting quota on
nested roles is disabled until MESOS-7402 can be fixed. This commit
disables any tests that rely on setting quota on nested roles; it also
adds a (disabled) test to cover the behavior that will be fixed as part
of MESOS-7402.
Diffs (updated)
-----
src/master/quota_handler.cpp 7ff43a048e17b9e9ac0ceed248f7b3fd56b007d6
src/tests/hierarchical_allocator_tests.cpp 33e7b455f8664858eb4f03727b076a10c80cd6e0
src/tests/master_quota_tests.cpp 1714ba13ea63bae05448d0898bf722ef472c672b
Diff: https://reviews.apache.org/r/58584/diff/3/
Changes: https://reviews.apache.org/r/58584/diff/2-3/
Testing
-------
`make check`
Thanks,
Neil Conway
Re: Review Request 58584: Disabled support for setting quota on
nested roles.
Posted by Neil Conway <ne...@gmail.com>.
> On April 21, 2017, 6:01 a.m., Jay Guo wrote:
> > src/master/quota_handler.cpp
> > Lines 518-521 (patched)
> > <https://reviews.apache.org/r/58584/diff/2/?file=1695640#file1695640line518>
> >
> > How about putting this before building `QuotaTree` so we don't bother to validate nested quota for now.
> >
> > Also, how about changing error message to something like: `"Setting quota on nested role '" + quotaInfo.role() + "' is not supported yet"`, so we don't mislead users to believe that we don't plan to.
Personally, I actually like validating quota for nested roles before returning the "not supported yet" error message: the quota validation code works fine and is not expensive, so I don't think we need to disable it.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58584/#review172587
-----------------------------------------------------------
On April 20, 2017, 6:18 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58584/
> -----------------------------------------------------------
>
> (Updated April 20, 2017, 6:18 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Correct support for quota on nested roles will require further work in
> the allocator (see MESOS-7402 for details). For now, setting quota on
> nested roles is disabled until MESOS-7402 can be fixed. This commit
> disables any tests that rely on setting quota on nested roles; it also
> adds a (disabled) test to cover the behavior that will be fixed as part
> of MESOS-7402.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 7ff43a048e17b9e9ac0ceed248f7b3fd56b007d6
> src/tests/hierarchical_allocator_tests.cpp 33e7b455f8664858eb4f03727b076a10c80cd6e0
> src/tests/master_quota_tests.cpp 1714ba13ea63bae05448d0898bf722ef472c672b
>
>
> Diff: https://reviews.apache.org/r/58584/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 58584: Disabled support for setting quota on
nested roles.
Posted by Jay Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58584/#review172587
-----------------------------------------------------------
LGTM
src/master/quota_handler.cpp
Lines 518-521 (patched)
<https://reviews.apache.org/r/58584/#comment245683>
How about putting this before building `QuotaTree` so we don't bother to validate nested quota for now.
Also, how about changing error message to something like: `"Setting quota on nested role '" + quotaInfo.role() + "' is not supported yet"`, so we don't mislead users to believe that we don't plan to.
src/tests/hierarchical_allocator_tests.cpp
Lines 4610 (patched)
<https://reviews.apache.org/r/58584/#comment245686>
How about `NON_QUOTA_ROLE` to be explicit.
src/tests/hierarchical_allocator_tests.cpp
Lines 4613 (patched)
<https://reviews.apache.org/r/58584/#comment245684>
I like indexing from zero too, but I think we start from one in most of the tests. Also the agent starts from one in this test case :P
src/tests/hierarchical_allocator_tests.cpp
Lines 4617 (patched)
<https://reviews.apache.org/r/58584/#comment245685>
Let's make value of mem greater than `MIN_MEM` to be consistent with other tests.
- Jay Guo
On April 21, 2017, 2:18 a.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58584/
> -----------------------------------------------------------
>
> (Updated April 21, 2017, 2:18 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Jay Guo, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Correct support for quota on nested roles will require further work in
> the allocator (see MESOS-7402 for details). For now, setting quota on
> nested roles is disabled until MESOS-7402 can be fixed. This commit
> disables any tests that rely on setting quota on nested roles; it also
> adds a (disabled) test to cover the behavior that will be fixed as part
> of MESOS-7402.
>
>
> Diffs
> -----
>
> src/master/quota_handler.cpp 7ff43a048e17b9e9ac0ceed248f7b3fd56b007d6
> src/tests/hierarchical_allocator_tests.cpp 33e7b455f8664858eb4f03727b076a10c80cd6e0
> src/tests/master_quota_tests.cpp 1714ba13ea63bae05448d0898bf722ef472c672b
>
>
> Diff: https://reviews.apache.org/r/58584/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>