Re: Review Request 39614: Quota: Added Status Validation Tests.

2016-01-05 Thread Joris Van Remoortere 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/#review112991
---

Ship it!


- Joris Van Remoortere


On Jan. 5, 2016, 6:25 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Jan. 5, 2016, 6:25 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-4218
> https://issues.apache.org/jira/browse/MESOS-4218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp 89130ce9b09afe7c6dd332c8b5278abe0d2674f1 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-12-22 Thread Joerg Schad 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/
---

(Updated Dec. 22, 2015, 10:19 a.m.)


Review request for mesos, Alexander Rukletsov and Bernd Mathiske.


Changes
---

Adressed comments


Bugs: MESOS-4218
https://issues.apache.org/jira/browse/MESOS-4218


Repository: mesos


Description
---

Quota: Added Status Validation Tests.


Diffs (updated)
-

  src/tests/master_quota_tests.cpp 89130ce9b09afe7c6dd332c8b5278abe0d2674f1 

Diff: https://reviews.apache.org/r/39614/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-12-22 Thread Joerg Schad 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/
---

(Updated Dec. 22, 2015, 9:40 a.m.)


Review request for mesos, Alexander Rukletsov and Bernd Mathiske.


Bugs: MESOS-4218
https://issues.apache.org/jira/browse/MESOS-4218


Repository: mesos


Description
---

Quota: Added Status Validation Tests.


Diffs
-

  src/tests/master_quota_tests.cpp 89130ce9b09afe7c6dd332c8b5278abe0d2674f1 

Diff: https://reviews.apache.org/r/39614/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-12-22 Thread Alexander Rukletsov 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/#review111597
---



src/tests/master_quota_tests.cpp (line 578)


The ReviewBot is failing because `quotaResources` must be flattened.

Also, why do you use `ASSERT_*` instead of `EXPECT_*` which are used 
throughout the test?


- Alexander Rukletsov


On Dec. 22, 2015, 10:19 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Dec. 22, 2015, 10:19 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-4218
> https://issues.apache.org/jira/browse/MESOS-4218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp 89130ce9b09afe7c6dd332c8b5278abe0d2674f1 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-12-22 Thread Alexander Rukletsov 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/#review111508
---


Looks good. Some high-level comments:
- You use `const` inconsistently. We have different opinions in the community 
about marking local variables `const`, I personally would like to see them more 
often. However, let's stay consistent throughout the test/function/class.
- I think we can easily merge two tests into one: query quota status; add an 
agent, set quota and query again; remove quota and query one more time. You can 
use scopes to separate these stages from one another like you did in 
`SetInvalidResourceInfos`. You can go even further and add status checks to 
existing tests, but I would prefer to have a separate test for status endpoint. 
I leave this decision to you and your shepherd.


src/tests/master_quota_tests.cpp (lines 492 - 493)


s/no quota set/no quotas set
s/"against /quota."/"."



src/tests/master_quota_tests.cpp (line 517)


Why `parse` is marked `const` and `array` isn't?



src/tests/master_quota_tests.cpp (line 540)


Since there is just one agent, no need to enumerate it.


- Alexander Rukletsov


On Dec. 21, 2015, 5:16 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Dec. 21, 2015, 5:16 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-4218
> https://issues.apache.org/jira/browse/MESOS-4218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp 89130ce9b09afe7c6dd332c8b5278abe0d2674f1 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-12-22 Thread Joerg Schad 


> On Dec. 22, 2015, 10:43 a.m., Alexander Rukletsov wrote:
> > src/tests/master_quota_tests.cpp, line 578
> > 
> >
> > The ReviewBot is failing because `quotaResources` must be flattened.
> > 
> > Also, why do you use `ASSERT_*` instead of `EXPECT_*` which are used 
> > throughout the test?

I use ASSERT_* if the following part of the test relies (hard) on the outcome, 
e.g. checking array sizes or calling get() on a Try.


- Joerg


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/#review111597
---


On Dec. 22, 2015, 10:19 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Dec. 22, 2015, 10:19 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-4218
> https://issues.apache.org/jira/browse/MESOS-4218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp 89130ce9b09afe7c6dd332c8b5278abe0d2674f1 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-12-22 Thread Joerg Schad 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/
---

(Updated Dec. 22, 2015, 12:18 p.m.)


Review request for mesos, Alexander Rukletsov and Bernd Mathiske.


Bugs: MESOS-4218
https://issues.apache.org/jira/browse/MESOS-4218


Repository: mesos


Description
---

Quota: Added Status Validation Tests.


Diffs (updated)
-

  src/tests/master_quota_tests.cpp 89130ce9b09afe7c6dd332c8b5278abe0d2674f1 

Diff: https://reviews.apache.org/r/39614/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-12-22 Thread Mesos ReviewBot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/#review111612
---


Patch looks great!

Reviews applied: [39492, 39614]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 22, 2015, 12:18 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Dec. 22, 2015, 12:18 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-4218
> https://issues.apache.org/jira/browse/MESOS-4218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp 89130ce9b09afe7c6dd332c8b5278abe0d2674f1 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-12-21 Thread Joerg Schad 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/
---

(Updated Dec. 21, 2015, 2:49 p.m.)


Review request for mesos, Alexander Rukletsov and Bernd Mathiske.


Changes
---

rebased


Bugs: MESOS-3073
https://issues.apache.org/jira/browse/MESOS-3073


Repository: mesos


Description
---

Quota: Added Status Validation Tests.


Diffs (updated)
-

  src/tests/master_quota_tests.cpp 89130ce9b09afe7c6dd332c8b5278abe0d2674f1 

Diff: https://reviews.apache.org/r/39614/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-12-21 Thread Joerg Schad 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/
---

(Updated Dec. 21, 2015, 5:13 p.m.)


Review request for mesos, Alexander Rukletsov and Bernd Mathiske.


Bugs: MESOS-3073
https://issues.apache.org/jira/browse/MESOS-3073


Repository: mesos


Description
---

Quota: Added Status Validation Tests.


Diffs
-

  src/tests/master_quota_tests.cpp 89130ce9b09afe7c6dd332c8b5278abe0d2674f1 

Diff: https://reviews.apache.org/r/39614/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-11-25 Thread Alexander Rukletsov 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/#review108008
---


Please adjust the JIRA ticket: MESOS-4013.

- Alexander Rukletsov


On Nov. 20, 2015, 10:05 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Nov. 20, 2015, 10:05 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-11-20 Thread Joerg Schad 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/
---

(Updated Nov. 20, 2015, 10:05 a.m.)


Review request for mesos, Alexander Rukletsov and Bernd Mathiske.


Changes
---

Correct ordering for expect call.


Bugs: MESOS-3073
https://issues.apache.org/jira/browse/MESOS-3073


Repository: mesos


Description
---

Quota: Added Status Validation Tests.


Diffs (updated)
-

  src/tests/master_quota_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/39614/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-11-19 Thread Joerg Schad 


> On Oct. 25, 2015, 3:51 a.m., Guangya Liu wrote:
> > src/tests/master_quota_tests.cpp, line 705
> > 
> >
> > I see that most of the test cases are not using xx_xx to name the test 
> > function, can we rename Status_EmptyQuota to StatusEmptyQuota? 
> > 
> > Same question for all of the functions in this file.

That was the old format :-). Updated now.


- Joerg


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/#review103933
---


On Oct. 24, 2015, 7:40 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Oct. 24, 2015, 7:40 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-11-19 Thread Joerg Schad 


> On Nov. 19, 2015, 1:55 p.m., Alexander Rukletsov wrote:
> > src/tests/master_quota_tests.cpp, line 724
> > 
> >
> > I can't understand, why `quota` and not `guarantees`. What am I missing?

The updated review :-).


- Joerg


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/#review107169
---


On Nov. 19, 2015, 2:48 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Nov. 19, 2015, 2:48 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-11-19 Thread Joerg Schad 


> On Nov. 6, 2015, 1:13 a.m., Joseph Wu wrote:
> > src/tests/master_quota_tests.cpp, lines 732-734
> > 
> >
> > Have you considered merging this test with 
> > `AvailableResourcesMultipleAgents`?
> 
> Joerg Schad wrote:
> In my opinion it is good to test both aspects seperatly from each other.
> 
> Alexander Rukletsov wrote:
> I think it's okay to merge as long as it's clear what fails. Same for 
> situations when no quota is set. Moreover, once we have a test with several 
> quotas, we can check status of multiple quotas as well.

I must say that alone from a grouping point I personally prefer to be able to 
easily execute find all Status related tests (and not having to remember that 
AvailableResourcesMultipleAgents also tests some functionality).


- Joerg


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/#review105343
---


On Nov. 19, 2015, 2:48 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Nov. 19, 2015, 2:48 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-11-19 Thread Joerg Schad 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/#review107313
---



src/tests/master_quota_tests.cpp (line 482)


Move Expect call before StartSlave()


- Joerg Schad


On Nov. 19, 2015, 4:05 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Nov. 19, 2015, 4:05 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-11-19 Thread Mesos ReviewBot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/#review107187
---


Patch looks great!

Reviews applied: [39211, 39018, 39102, 36913, 38059, 39285, 38110, 40342, 
40351, 38956, 40396, 39223, 39492, 39614]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Nov. 19, 2015, 2:48 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Nov. 19, 2015, 2:48 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-11-19 Thread Joerg Schad 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/
---

(Updated Nov. 19, 2015, 4:05 p.m.)


Review request for mesos, Alexander Rukletsov and Bernd Mathiske.


Bugs: MESOS-3073
https://issues.apache.org/jira/browse/MESOS-3073


Repository: mesos


Description
---

Quota: Added Status Validation Tests.


Diffs (updated)
-

  src/tests/master_quota_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/39614/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-11-19 Thread Alexander Rukletsov 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/#review107181
---



src/tests/master_quota_tests.cpp (line 435)


let's wrap "from" to avoid jaggedness.



src/tests/master_quota_tests.cpp (line 443)


Don't we need to authenticate?



src/tests/master_quota_tests.cpp (lines 455 - 456)


It feels like this test may not test what it should. The field may miss for 
two reasons: no quotas set _or_ there is no such field at all : ).

I'd suggest to either convert to the protobuf and then check the size of 
the collection, or check against entire `JSON` object.



src/tests/master_quota_tests.cpp (line 479)


indent



src/tests/master_quota_tests.cpp (lines 479 - 480)


blank line



src/tests/master_quota_tests.cpp (line 490)


space



src/tests/master_quota_tests.cpp (lines 490 - 491)


blank line



src/tests/master_quota_tests.cpp (lines 494 - 497)


Let's add a check that total cluster resources contain `quotaResources`.



src/tests/master_quota_tests.cpp (lines 495 - 496)


Fits one line



src/tests/master_quota_tests.cpp (line 496)


Please use constant `ROLE1`



src/tests/master_quota_tests.cpp (lines 500 - 503)


indentation



src/tests/master_quota_tests.cpp (line 520)


How about "Convert `JSON` response to `QuotaStatus` protobuf"?

Also, mind backticks.



src/tests/master_quota_tests.cpp (line 521)


`protobuf::parse()` does not compile?



src/tests/master_quota_tests.cpp (line 524)


`RepeatedField::size()` returns `int`


- Alexander Rukletsov


On Nov. 19, 2015, 2:48 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Nov. 19, 2015, 2:48 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-11-19 Thread Joerg Schad 


> On Nov. 19, 2015, 3:34 p.m., Alexander Rukletsov wrote:
> > src/tests/master_quota_tests.cpp, line 521
> > 
> >
> > `protobuf::parse()` does not compile?

No.


- Joerg


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/#review107181
---


On Nov. 19, 2015, 2:48 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Nov. 19, 2015, 2:48 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-11-19 Thread Joerg Schad 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/
---

(Updated Nov. 19, 2015, 2:48 p.m.)


Review request for mesos, Alexander Rukletsov and Bernd Mathiske.


Bugs: MESOS-3073
https://issues.apache.org/jira/browse/MESOS-3073


Repository: mesos


Description
---

Quota: Added Status Validation Tests.


Diffs (updated)
-

  src/tests/master_quota_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/39614/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-11-19 Thread Alexander Rukletsov 


> On Nov. 6, 2015, 1:13 a.m., Joseph Wu wrote:
> > src/tests/master_quota_tests.cpp, lines 732-734
> > 
> >
> > Have you considered merging this test with 
> > `AvailableResourcesMultipleAgents`?
> 
> Joerg Schad wrote:
> In my opinion it is good to test both aspects seperatly from each other.

I think it's okay to merge as long as it's clear what fails. Same for 
situations when no quota is set. Moreover, once we have a test with several 
quotas, we can check status of multiple quotas as well.


- Alexander


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/#review105343
---


On Nov. 19, 2015, 2:48 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Nov. 19, 2015, 2:48 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-11-19 Thread Alexander Rukletsov 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/#review107169
---



src/tests/master_quota_tests.cpp (line 724)


I can't understand, why `quota` and not `guarantees`. What am I missing?



src/tests/master_quota_tests.cpp (line 794)


Let's expand this test beyond a simple size check.


- Alexander Rukletsov


On Oct. 24, 2015, 7:40 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Oct. 24, 2015, 7:40 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-11-19 Thread Alexander Rukletsov 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/#review107159
---



src/tests/master_quota_tests.cpp (line 701)


Why do you call them status validation tests? Do they belong to this test 
section?



src/tests/master_quota_tests.cpp (line 703)


3rd person singular, please.



src/tests/master_quota_tests.cpp (lines 703 - 705)


I'd say "empty quota" is a bit ambiguous. How about `StatusNoQuotas`?



src/tests/master_quota_tests.cpp (line 712)


Do you want to check status here as well?



src/tests/master_quota_tests.cpp (lines 785 - 787)


If we create an implicit contract that the status JSON is based on 
`QuotaStatus` proto—which I think is a good idea—we should maybe check we can 
convert the `JSON` to proto.


- Alexander Rukletsov


On Oct. 24, 2015, 7:40 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Oct. 24, 2015, 7:40 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-11-19 Thread Joerg Schad 


> On Nov. 6, 2015, 1:13 a.m., Joseph Wu wrote:
> > src/tests/master_quota_tests.cpp, lines 732-734
> > 
> >
> > Have you considered merging this test with 
> > `AvailableResourcesMultipleAgents`?

In my opinion it is good to test both aspects seperatly from each other.


- Joerg


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/#review105343
---


On Oct. 24, 2015, 7:40 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Oct. 24, 2015, 7:40 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-11-19 Thread Mesos ReviewBot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/#review107203
---


Patch looks great!

Reviews applied: [39211, 39018, 39102, 36913, 38059, 39285, 38110, 40342, 
40351, 38956, 40396, 39223, 39492, 39614]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Nov. 19, 2015, 4:05 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Nov. 19, 2015, 4:05 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-11-05 Thread Joseph Wu 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/#review105343
---



src/tests/master_quota_tests.cpp (lines 723 - 726)


The comment you have here seems incorrect.



src/tests/master_quota_tests.cpp (lines 732 - 734)


Have you considered merging this test with 
`AvailableResourcesMultipleAgents`?



src/tests/master_quota_tests.cpp (line 790)


This comment is also out-of-sync.


- Joseph Wu


On Oct. 24, 2015, 12:40 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Oct. 24, 2015, 12:40 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-10-24 Thread Joerg Schad 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/
---

(Updated Oct. 24, 2015, 7:40 p.m.)


Review request for mesos, Alexander Rukletsov and Bernd Mathiske.


Bugs: MESOS-3073
https://issues.apache.org/jira/browse/MESOS-3073


Repository: mesos


Description
---

Quota: Added Status Validation Tests.


Diffs (updated)
-

  src/tests/master_quota_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/39614/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-10-24 Thread Mesos ReviewBot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/#review103907
---


Patch looks great!

Reviews applied: [39317, 38218, 36913, 38059, 39285, 38110, 38956, 39223, 
39492, 39614]

All tests passed.

- Mesos ReviewBot


On Oct. 24, 2015, 7:40 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Oct. 24, 2015, 7:40 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-10-24 Thread Guangya Liu 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/#review103933
---



src/tests/master_quota_tests.cpp (line 705)


I see that most of the test cases are not using xx_xx to name the test 
function, can we rename Status_EmptyQuota to StatusEmptyQuota? 

Same question for all of the functions in this file.


- Guangya Liu


On 十月 24, 2015, 7:40 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated 十月 24, 2015, 7:40 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39614: Quota: Added Status Validation Tests.

2015-10-23 Thread Mesos ReviewBot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/#review103890
---


Patch looks great!

Reviews applied: [39317, 38218, 36913, 38059, 39285, 38110, 38956, 39223, 
39492, 39614]

All tests passed.

- Mesos ReviewBot


On Oct. 23, 2015, 10:35 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Oct. 23, 2015, 10:35 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Review Request 39614: Quota: Added Status Validation Tests.

2015-10-23 Thread Joerg Schad 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39614/
---

Review request for mesos, Alexander Rukletsov and Bernd Mathiske.


Bugs: MESOS-3073
https://issues.apache.org/jira/browse/MESOS-3073


Repository: mesos


Description
---

Quota: Added Status Validation Tests.


Diffs
-

  src/tests/master_quota_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/39614/diff/


Testing
---

make check


Thanks,

Joerg Schad