You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Zhitao Li <zh...@gmail.com> on 2017/08/05 04:38:47 UTC
Re: Review Request 52284: Implemented more quota validation tests and
validate duplicate name.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52284/
-----------------------------------------------------------
(Updated Aug. 5, 2017, 4:38 a.m.)
Review request for mesos, Alexander Rukletsov, Xiaojian Huang, and Neil Conway.
Changes
-------
Rebase recent changes.
Bugs: MESOS-4292
https://issues.apache.org/jira/browse/MESOS-4292
Repository: mesos
Description
-------
Implemented more quota validation tests and validate duplicate name.
Diffs (updated)
-----
src/master/quota.cpp 79e4ad8ae5cdada8ac98d663b50645ae0e792f72
src/tests/master_quota_tests.cpp 9d52f767c112abd55ab3f49d172eb6e3caea511b
Diff: https://reviews.apache.org/r/52284/diff/7/
Changes: https://reviews.apache.org/r/52284/diff/6-7/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 52284: Implemented more quota validation tests and
validate duplicate name.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52284/#review186471
-----------------------------------------------------------
Fix it, then Ship it!
Thanks for the patch, it looks like this needs to be split apart since there is an unrelated test being added?
I will pull out the unrelated changes and get the new validation committed for you.
src/master/quota.cpp
Lines 156-158 (patched)
<https://reviews.apache.org/r/52284/#comment263036>
"Repeat twice" sounds like 3 occurrences? Would be good to re-phrase to be less ambiguous, like "Check that the resource names are unique" or "Check for duplicate resource names".
src/master/quota.cpp
Lines 158 (patched)
<https://reviews.apache.org/r/52284/#comment263024>
Let's include the name here.
src/master/quota.cpp
Lines 159-161 (patched)
<https://reviews.apache.org/r/52284/#comment263025>
Can you remove the else?
src/tests/master_quota_tests.cpp
Lines 158-162 (original)
<https://reviews.apache.org/r/52284/#comment263032>
The last test is added in this review, but why did you remove the others? Are they already present? Deserves its own patch since it's not related to validating unique resource names.
src/tests/master_quota_tests.cpp
Lines 160-184 (patched)
<https://reviews.apache.org/r/52284/#comment263030>
This looks like it deserves its own patch? It's not related to your change to validate unique resource names. Also, as far as I can tell, it's already tested?
src/tests/master_quota_tests.cpp
Line 166 (original), 189 (patched)
<https://reviews.apache.org/r/52284/#comment263039>
I was initially confused about implicit role vs non-existent role. Re-naming this to "NonWhitelistedRole" would be a bit clearer to me.
src/tests/master_quota_tests.cpp
Line 192 (original), 215 (patched)
<https://reviews.apache.org/r/52284/#comment263026>
Whoops? This looks unrelated.
src/tests/master_quota_tests.cpp
Lines 366 (patched)
<https://reviews.apache.org/r/52284/#comment263037>
s/.get()./->/
- Benjamin Mahler
On Aug. 5, 2017, 4:38 a.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52284/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2017, 4:38 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Xiaojian Huang, and Neil Conway.
>
>
> Bugs: MESOS-4292
> https://issues.apache.org/jira/browse/MESOS-4292
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented more quota validation tests and validate duplicate name.
>
>
> Diffs
> -----
>
> src/master/quota.cpp 79e4ad8ae5cdada8ac98d663b50645ae0e792f72
> src/tests/master_quota_tests.cpp 9d52f767c112abd55ab3f49d172eb6e3caea511b
>
>
> Diff: https://reviews.apache.org/r/52284/diff/7/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>