Re: Review Request 58584: Disabled support for setting quota on nested roles.

2017-04-24 Thread Neil Conway 

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

2017-04-24 Thread Neil Conway 


> On April 21, 2017, 6:01 a.m., Jay Guo wrote:
> > src/master/quota_handler.cpp
> > Lines 518-521 (patched)
> > 
> >
> > 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.

2017-04-21 Thread Jay Guo 

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


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)


How about `NON_QUOTA_ROLE` to be explicit.



src/tests/hierarchical_allocator_tests.cpp
Lines 4613 (patched)


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)


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

Re: Review Request 58584: Disabled support for setting quota on nested roles.

2017-04-20 Thread Neil Conway 

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