Re: Review Request 52284: Implemented more quota validation tests and validate duplicate name.

[Benjamin Mahler]

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


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


Let's include the name here.



src/master/quota.cpp
Lines 159-161 (patched)


Can you remove the else?



src/tests/master_quota_tests.cpp
Lines 158-162 (original)


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)


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)


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)


Whoops? This looks unrelated.



src/tests/master_quota_tests.cpp
Lines 366 (patched)


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.

[Zhitao Li]

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

[Zhitao Li]

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

[Zhitao Li]

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

[Benjamin Bannier]

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

[Zhitao Li]

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

[Zhitao Li]

> On Feb. 8, 2017, 10:41 a.m., Benjamin Bannier wrote:
> > src/tests/master_quota_tests.cpp, line 390
> > 
> >
> > 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 and validate duplicate name.

[Zhitao Li]

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

[Benjamin Bannier]

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


You could use `UNKNOWN_RULE` here to make this more self-documenting.



src/tests/master_quota_tests.cpp (line 231)


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)


Please add an expectation for the response body.



src/tests/master_quota_tests.cpp (line 255)


Please add an expectation for the response body.



src/tests/master_quota_tests.cpp (line 390)


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)


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.

[Zhitao Li]

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