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/02/09 21:40:10 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 Feb. 9, 2017, 9:40 p.m.)
Review request for mesos, Alexander Rukletsov and Xiaojian Huang.
Changes
-------
Review comments.
Summary (updated)
-----------------
Implemented more quota validation tests and validate duplicate name.
Bugs: MESOS-4292
https://issues.apache.org/jira/browse/MESOS-4292
Repository: mesos
Description (updated)
-------
Implemented more quota validation tests and validate duplicate name.
Diffs (updated)
-----
src/master/quota.cpp 847ec068f4f3fe94ddabc11ff583d7e76117d375
src/tests/master_quota_tests.cpp b25f5911579c435549b9bc65994627414357dcb6
Diff: https://reviews.apache.org/r/52284/diff/
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
>
>
Re: Review Request 52284: Implemented more quota validation tests and
validate duplicate name.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
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 Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52284/
-----------------------------------------------------------
(Updated Feb. 13, 2017, 7:07 p.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 (updated)
-----
src/master/quota.cpp 847ec068f4f3fe94ddabc11ff583d7e76117d375
src/tests/master_quota_tests.cpp d15c7aacb85596cdee7cf59c0c179247ba624fe5
Diff: https://reviews.apache.org/r/52284/diff/
Testing
-------
Thanks,
Zhitao Li
Re: Review Request 52284: Implemented more quota validation tests and
validate duplicate name.
Posted by Zhitao Li <zh...@gmail.com>.
> On Feb. 13, 2017, 4:37 p.m., Benjamin Bannier wrote:
> > Thanks for adding validation of the error messages.
> >
> > Currently you output the response body should it not have the right status code. I think the following would make more sense:
> >
> > AWAIT_EXPECT_RESPONSE_STATUS_EQ(BadRequest().status, response);
> >
> > AWAIT_EXPECT_RESPONSE_BODY_EQ(
> > "Failed to validate set quota request: QuotaInfo may not contain same "
> > "resource name twice",
> > response)
> > << response.get().body;
DONE.
- Zhitao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52284/#review165352
-----------------------------------------------------------
On Feb. 13, 2017, 7:07 p.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52284/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2017, 7:07 p.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 847ec068f4f3fe94ddabc11ff583d7e76117d375
> src/tests/master_quota_tests.cpp d15c7aacb85596cdee7cf59c0c179247ba624fe5
>
> Diff: https://reviews.apache.org/r/52284/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 52284: Implemented more quota validation tests and
validate duplicate name.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52284/#review165352
-----------------------------------------------------------
Thanks for adding validation of the error messages.
Currently you output the response body should it not have the right status code. I think the following would make more sense:
AWAIT_EXPECT_RESPONSE_STATUS_EQ(BadRequest().status, response);
AWAIT_EXPECT_RESPONSE_BODY_EQ(
"Failed to validate set quota request: QuotaInfo may not contain same "
"resource name twice",
response)
<< response.get().body;
- Benjamin Bannier
On Feb. 10, 2017, 8:54 a.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52284/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2017, 8:54 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 847ec068f4f3fe94ddabc11ff583d7e76117d375
> src/tests/master_quota_tests.cpp d15c7aacb85596cdee7cf59c0c179247ba624fe5
>
> Diff: https://reviews.apache.org/r/52284/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 52284: Implemented more quota validation tests and
validate duplicate name.
Posted by Zhitao Li <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52284/
-----------------------------------------------------------
(Updated Feb. 10, 2017, 7:54 a.m.)
Review request for mesos, Alexander Rukletsov, Xiaojian Huang, and Neil Conway.
Changes
-------
Rebase after recent change.
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 847ec068f4f3fe94ddabc11ff583d7e76117d375
src/tests/master_quota_tests.cpp d15c7aacb85596cdee7cf59c0c179247ba624fe5
Diff: https://reviews.apache.org/r/52284/diff/
Testing
-------
Thanks,
Zhitao Li