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/07 07:10:49 UTC
Re: Review Request 52284: Implemented more quota validation tests.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52284/
-----------------------------------------------------------
(Updated Feb. 7, 2017, 7:10 a.m.)
Review request for mesos, Alexander Rukletsov and Xiaojian Huang.
Changes
-------
Rebase and review comments.
Summary (updated)
-----------------
Implemented more quota validation tests.
Bugs: MESOS-4292
https://issues.apache.org/jira/browse/MESOS-4292
Repository: mesos
Description (updated)
-------
Implemented more quota validation tests.
Diffs (updated)
-----
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 Zhitao Li <zh...@gmail.com>.
> On Feb. 8, 2017, 10:41 a.m., Benjamin Bannier wrote:
> > src/tests/master_quota_tests.cpp, line 390
> > <https://reviews.apache.org/r/52284/diff/3/?file=1626117#file1626117line390>
> >
> > Should we explicitly assert here that this isn't parsed as `cpus:2;mem:512`, e.g.,
> >
> > ASSERT_EQ(3u, quotaResources.size());
Great catch. This actually didn't verify the case it's guarding against, because we allowed it.
I've added this validation in this patch now and made this correctly rejected.
- Zhitao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52284/#review164646
-----------------------------------------------------------
On Feb. 9, 2017, 9:40 p.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> 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.
>
>
> 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 b25f5911579c435549b9bc65994627414357dcb6
>
> Diff: https://reviews.apache.org/r/52284/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Zhitao Li
>
>
Re: Review Request 52284: Implemented more quota validation tests.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52284/#review164646
-----------------------------------------------------------
src/tests/master_quota_tests.cpp (line 174)
<https://reviews.apache.org/r/52284/#comment236411>
You could use `UNKNOWN_RULE` here to make this more self-documenting.
src/tests/master_quota_tests.cpp (line 231)
<https://reviews.apache.org/r/52284/#comment236407>
This is not very resilent should we introduce other validation rejecting this request for other reasons. Could you add an expectation for the response body?
src/tests/master_quota_tests.cpp (line 243)
<https://reviews.apache.org/r/52284/#comment236408>
Please add an expectation for the response body.
src/tests/master_quota_tests.cpp (line 255)
<https://reviews.apache.org/r/52284/#comment236409>
Please add an expectation for the response body.
src/tests/master_quota_tests.cpp (line 390)
<https://reviews.apache.org/r/52284/#comment236412>
Should we explicitly assert here that this isn't parsed as `cpus:2;mem:512`, e.g.,
ASSERT_EQ(3u, quotaResources.size());
src/tests/master_quota_tests.cpp (line 394)
<https://reviews.apache.org/r/52284/#comment236410>
Please add an expectation for the response body.
- Benjamin Bannier
On Feb. 7, 2017, 8:10 a.m., Zhitao Li wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52284/
> -----------------------------------------------------------
>
> (Updated Feb. 7, 2017, 8:10 a.m.)
>
>
> Review request for mesos, Alexander Rukletsov and Xiaojian Huang.
>
>
> Bugs: MESOS-4292
> https://issues.apache.org/jira/browse/MESOS-4292
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented more quota validation tests.
>
>
> Diffs
> -----
>
> 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
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. 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