Re: Review Request 42588: Added unit-test for `NetClsHandleManager`.

2016-02-05 Thread Jie Yu 

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 171 - 173)


Why error here? THis should be false, right? I'll fix it for you.


- Jie Yu


On Feb. 5, 2016, 2:16 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42588/
> ---
> 
> (Updated Feb. 5, 2016, 2:16 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit-test for `NetClsHandleManager`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
> 85004aea4c1ee4b25e106f3ce40025c69f1ce030 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
> bf6c88341dedc0a37546c04f38197c892b498684 
>   src/tests/containerizer/isolator_tests.cpp 
> 67322abc776cbd501385932676852a79b74ef248 
> 
> Diff: https://reviews.apache.org/r/42588/diff/
> 
> 
> Testing
> ---
> 
> make and make check.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 42588: Added unit-test for `NetClsHandleManager`.

2016-02-04 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [42586, 42587, 42588]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 5, 2016, 2:16 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42588/
> ---
> 
> (Updated Feb. 5, 2016, 2:16 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit-test for `NetClsHandleManager`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
> 85004aea4c1ee4b25e106f3ce40025c69f1ce030 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
> bf6c88341dedc0a37546c04f38197c892b498684 
>   src/tests/containerizer/isolator_tests.cpp 
> 67322abc776cbd501385932676852a79b74ef248 
> 
> Diff: https://reviews.apache.org/r/42588/diff/
> 
> 
> Testing
> ---
> 
> make and make check.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 42588: Added unit-test for `NetClsHandleManager`.

2016-02-04 Thread Avinash sridharan 

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

(Updated Feb. 5, 2016, 2:16 a.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added unit-test for `NetClsHandleManager`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
85004aea4c1ee4b25e106f3ce40025c69f1ce030 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
bf6c88341dedc0a37546c04f38197c892b498684 
  src/tests/containerizer/isolator_tests.cpp 
67322abc776cbd501385932676852a79b74ef248 

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


Testing
---

make and make check.


Thanks,

Avinash sridharan

Re: Review Request 42588: Added unit-test for `NetClsHandleManager`.

2016-02-04 Thread Avinash sridharan 


> On Feb. 5, 2016, 12:04 a.m., Jie Yu wrote:
> > src/tests/containerizer/isolator_tests.cpp, line 382
> > 
> >
> > IntervalSet here is not needed. Just do
> > 
> > ```
> > NetClsHandleManager manager(
> > (Bound::closed(3),
> >  Bound::closed(3));
> >  
> > 
> > ```

As discussed need IntervalSet due to explicit constructor in 
IntervalSet


> On Feb. 5, 2016, 12:04 a.m., Jie Yu wrote:
> > src/tests/containerizer/isolator_tests.cpp, line 405
> > 
> >
> > Ditto.

Same reason as above.


> On Feb. 5, 2016, 12:04 a.m., Jie Yu wrote:
> > src/tests/containerizer/isolator_tests.cpp, line 417
> > 
> >
> > Ditto.

Same reason as above.


- Avinash


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


On Feb. 4, 2016, 9:03 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42588/
> ---
> 
> (Updated Feb. 4, 2016, 9:03 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit-test for `NetClsHandleManager`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
> b4bc52114389d1c1efce2830f4292bd89bb0de7c 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
> ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 
>   src/tests/containerizer/isolator_tests.cpp 
> 8d101df957fd36adac388310eddba2db1f98c029 
> 
> Diff: https://reviews.apache.org/r/42588/diff/
> 
> 
> Testing
> ---
> 
> make and make check.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 42588: Added unit-test for `NetClsHandleManager`.

2016-02-04 Thread Jie Yu 

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


Fix it, then Ship it!





src/tests/containerizer/isolator_tests.cpp (line 374)


You don't need to inherit from MesosTest for this. Just inherit from 
::testing::Test



src/tests/containerizer/isolator_tests.cpp (lines 377 - 379)


Can you move this, along with NetClsIsolatorTest to a separate file (e.g., 
cgroups_net_cls_isolator_tests.cpp)



src/tests/containerizer/isolator_tests.cpp (line 382)


IntervalSet here is not needed. Just do

```
NetClsHandleManager manager(
(Bound::closed(3),
 Bound::closed(3));
 

```



src/tests/containerizer/isolator_tests.cpp (line 405)


Ditto.



src/tests/containerizer/isolator_tests.cpp (line 417)


Ditto.


- Jie Yu


On Feb. 4, 2016, 9:03 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42588/
> ---
> 
> (Updated Feb. 4, 2016, 9:03 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit-test for `NetClsHandleManager`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
> b4bc52114389d1c1efce2830f4292bd89bb0de7c 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
> ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 
>   src/tests/containerizer/isolator_tests.cpp 
> 8d101df957fd36adac388310eddba2db1f98c029 
> 
> Diff: https://reviews.apache.org/r/42588/diff/
> 
> 
> Testing
> ---
> 
> make and make check.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 42588: Added unit-test for `NetClsHandleManager`.

2016-02-04 Thread Avinash sridharan 


> On Feb. 4, 2016, 7:21 p.m., Jie Yu wrote:
> > src/tests/containerizer/isolator_tests.cpp, lines 374-395
> > 
> >
> > Hum, I don't think this test fixture is needed. Just create 
> > handleManager in each test.
> > 
> > ```
> > NetClsHandleManager manager(
> > (Bound::closed(2),
> >  Bound::closed(3));
> >  
> > for (int primary = 2; primary <= 3; primary++) {
> >   ...
> > }
> > 
> > for (int primary = 2; primary <= 3; primary++) {
> >   ...
> > }
> > ```

Couldn't use (Bound::closed(2),
 Bound::closed(3) had to use 
Intervalset((Bound::closed(2),
 Bound::closed(3))


> On Feb. 4, 2016, 7:21 p.m., Jie Yu wrote:
> > src/tests/containerizer/isolator_tests.cpp, lines 411-412
> > 
> >
> > These two can be combined into:
> > ```
> > EXPECT_SOME_TRUE(manager->isUsed(handle.get());
> > ```

Thanks !! Simplified.


- Avinash


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


On Feb. 4, 2016, 9:03 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42588/
> ---
> 
> (Updated Feb. 4, 2016, 9:03 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit-test for `NetClsHandleManager`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
> b4bc52114389d1c1efce2830f4292bd89bb0de7c 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
> ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 
>   src/tests/containerizer/isolator_tests.cpp 
> 8d101df957fd36adac388310eddba2db1f98c029 
> 
> Diff: https://reviews.apache.org/r/42588/diff/
> 
> 
> Testing
> ---
> 
> make and make check.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 42588: Added unit-test for `NetClsHandleManager`.

2016-02-04 Thread Avinash sridharan 

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

(Updated Feb. 4, 2016, 9:03 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added unit-test for `NetClsHandleManager`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
b4bc52114389d1c1efce2830f4292bd89bb0de7c 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 
  src/tests/containerizer/isolator_tests.cpp 
8d101df957fd36adac388310eddba2db1f98c029 

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


Testing
---

make and make check.


Thanks,

Avinash sridharan

Re: Review Request 42588: Added unit-test for `NetClsHandleManager`.

2016-02-04 Thread Jie Yu 

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




src/tests/containerizer/isolator_tests.cpp (lines 374 - 395)


Hum, I don't think this test fixture is needed. Just create handleManager 
in each test.

```
NetClsHandleManager manager(
(Bound::closed(2),
 Bound::closed(3));
 
for (int primary = 2; primary <= 3; primary++) {
  ...
}

for (int primary = 2; primary <= 3; primary++) {
  ...
}
```



src/tests/containerizer/isolator_tests.cpp (lines 411 - 412)


These two can be combined into:
```
EXPECT_SOME_TRUE(manager->isUsed(handle.get());
```



src/tests/containerizer/isolator_tests.cpp (lines 414 - 415)


Hum, this is too implicit. Why do you need the inner loop?

In fact, I would suggest that we don't use the outer loop as well. Just 
test the single primary case.



src/tests/containerizer/isolator_tests.cpp (line 443)


No need for the loop here. Just pick a primary that's not within the range.



src/tests/containerizer/isolator_tests.cpp (line 452)


No need for the loop here. just pick a primary.



src/tests/containerizer/isolator_tests.cpp (line 453)


No need for this as well. Just pick a secondary.


- Jie Yu


On Feb. 4, 2016, 6:18 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42588/
> ---
> 
> (Updated Feb. 4, 2016, 6:18 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit-test for `NetClsHandleManager`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
> b4bc52114389d1c1efce2830f4292bd89bb0de7c 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
> ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 
>   src/tests/containerizer/isolator_tests.cpp 
> 8d101df957fd36adac388310eddba2db1f98c029 
> 
> Diff: https://reviews.apache.org/r/42588/diff/
> 
> 
> Testing
> ---
> 
> make and make check.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 42588: Added unit-test for `NetClsHandleManager`.

2016-02-04 Thread Avinash sridharan 

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

(Updated Feb. 4, 2016, 6:18 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Enhanced the error message returned from isUsed.


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


Repository: mesos


Description
---

Added unit-test for `NetClsHandleManager`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
b4bc52114389d1c1efce2830f4292bd89bb0de7c 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 
  src/tests/containerizer/isolator_tests.cpp 
8d101df957fd36adac388310eddba2db1f98c029 

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


Testing
---

make and make check.


Thanks,

Avinash sridharan

Re: Review Request 42588: Added unit-test for `NetClsHandleManager`.

2016-02-04 Thread Avinash sridharan 


> On Feb. 3, 2016, 6:18 p.m., Jie Yu wrote:
> > src/tests/containerizer/isolator_tests.cpp, line 379
> > 
> >
> > I would suggest we create smaller tests, each of which test one 
> > specific part of the code. For intance, the first test can just do an alloc 
> > on a primary and make sure it's used (isUsed). The second test tests the 
> > free. In other words, do an alloc and a free, make sure isUsed returns 
> > false. Finally, test the full case, make sure alloc returns an Error.

Broke it into 3 tests. AllocateFreeHandles (Tests allocate and free), 
AllocateInvalidPrimary (tests for invalid primaries), ReserveHandles (tests 
reservations).


- Avinash


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


On Feb. 4, 2016, 5:39 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42588/
> ---
> 
> (Updated Feb. 4, 2016, 5:39 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit-test for `NetClsHandleManager`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
> b4bc52114389d1c1efce2830f4292bd89bb0de7c 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
> ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 
>   src/tests/containerizer/isolator_tests.cpp 
> 8d101df957fd36adac388310eddba2db1f98c029 
> 
> Diff: https://reviews.apache.org/r/42588/diff/
> 
> 
> Testing
> ---
> 
> make and make check.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 42588: Added unit-test for `NetClsHandleManager`.

2016-02-04 Thread Avinash sridharan 

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

(Updated Feb. 4, 2016, 5:39 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added unit-test for `NetClsHandleManager`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
b4bc52114389d1c1efce2830f4292bd89bb0de7c 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 
  src/tests/containerizer/isolator_tests.cpp 
8d101df957fd36adac388310eddba2db1f98c029 

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


Testing
---

make and make check.


Thanks,

Avinash sridharan

Re: Review Request 42588: Added unit-test for `NetClsHandleManager`.

2016-02-03 Thread Jie Yu 

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




src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 176)


Why Result here? When None() will be returned? Should this just be a Try 
here?



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 182)


Ditto on using explicit check.



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (line 187)


No need for this temp variable.



src/tests/containerizer/isolator_tests.cpp (line 379)


I would suggest we create smaller tests, each of which test one specific 
part of the code. For intance, the first test can just do an alloc on a primary 
and make sure it's used (isUsed). The second test tests the free. In other 
words, do an alloc and a free, make sure isUsed returns false. Finally, test 
the full case, make sure alloc returns an Error.



src/tests/containerizer/isolator_tests.cpp (lines 398 - 399)


No need for error message for assertions/expectations. Simply do:
```
ASSERT_SOME(handle);
```



src/tests/containerizer/isolator_tests.cpp (lines 404 - 407)


Kill.



src/tests/containerizer/isolator_tests.cpp (lines 409 - 411)


Kill.


- Jie Yu


On Feb. 3, 2016, 7:11 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42588/
> ---
> 
> (Updated Feb. 3, 2016, 7:11 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit-test for `NetClsHandleManager`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
> b4bc52114389d1c1efce2830f4292bd89bb0de7c 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
> ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 
>   src/tests/containerizer/isolator_tests.cpp 
> 8d101df957fd36adac388310eddba2db1f98c029 
> 
> Diff: https://reviews.apache.org/r/42588/diff/
> 
> 
> Testing
> ---
> 
> make and make check.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 42588: Added unit-test for `NetClsHandleManager`.

2016-02-02 Thread Avinash sridharan 

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

(Updated Feb. 3, 2016, 7:11 a.m.)


Review request for mesos and Jie Yu.


Summary (updated)
-

Added unit-test for `NetClsHandleManager`.


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


Repository: mesos


Description (updated)
---

Added unit-test for `NetClsHandleManager`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
b4bc52114389d1c1efce2830f4292bd89bb0de7c 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 
  src/tests/containerizer/isolator_tests.cpp 
8d101df957fd36adac388310eddba2db1f98c029 

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


Testing
---

make and make check.


Thanks,

Avinash sridharan