Re: Review Request 42587: Implemented the `NetClsHandleMgr` class.

2016-02-02 Thread Avinash sridharan 

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

(Updated Feb. 2, 2016, 6:16 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Implemented the `NetClsHandleMgr` class.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
b4bc52114389d1c1efce2830f4292bd89bb0de7c 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 

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


Testing
---

make


Thanks,

Avinash sridharan

Re: Review Request 42587: Implemented the `NetClsHandleMgr` class.

2016-02-02 Thread Avinash sridharan 


> On Feb. 1, 2016, 10:46 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp, lines 137-139
> > 
> >
> > Hum, having two data fields here looks a little confusing to me. How 
> > about just merging them:
> > 
> > ```
> > // A map for used handles: primary -> bitset for secondary.
> > hashmap> used;
> > ```

Used a typedef for bitset<0x+1> to improve readability.


> On Feb. 1, 2016, 10:46 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp, lines 73-78
> > 
> >
> > I got confused here. minor is a uint16 value. Why this check is 
> > necessary?

Removed this validation. Always set the first element of the bitmask when 
initializing it, so no longer need the check on the secondary handles.


> On Feb. 1, 2016, 10:46 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp, lines 82-96
> > 
> >
> > For testin, I would probably just expose a 'isUsed' method:
> > ```
> > bool isUsed(const NetClsHandle& handle);
> > ```

Removed usage(). Will introduce isUsed with the unit-test patch.


- Avinash


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


On Feb. 2, 2016, 6:16 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42587/
> ---
> 
> (Updated Feb. 2, 2016, 6:16 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented the `NetClsHandleMgr` class.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
> b4bc52114389d1c1efce2830f4292bd89bb0de7c 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
> ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 
> 
> Diff: https://reviews.apache.org/r/42587/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 42587: Implemented the `NetClsHandleMgr` class.

2016-02-02 Thread Avinash sridharan 

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

(Updated Feb. 3, 2016, 12:44 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Implemented the `NetClsHandleMgr` class.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
b4bc52114389d1c1efce2830f4292bd89bb0de7c 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 

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


Testing
---

make


Thanks,

Avinash sridharan

Re: Review Request 42587: Implemented the `NetClsHandleMgr` class.

2016-02-01 Thread Jie Yu 

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




src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp (lines 137 - 139)


Hum, having two data fields here looks a little confusing to me. How about 
just merging them:

```
// A map for used handles: primary -> bitset for secondary.
hashmap> used;
```



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


Please insert a blank line above.



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 53 - 62)


This is a simple 'contains' check. For readability, I would rather inline 
this instead of creating a helper method.



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 73 - 78)


I got confused here. minor is a uint16 value. Why this check is necessary?



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 82 - 96)


For testin, I would probably just expose a 'isUsed' method:
```
bool isUsed(const NetClsHandle& handle);
```



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 102 - 105)


See my comments above. I would rather have the 'contains' check here.



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


why minorHandle -1 here?



src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp (lines 114 - 116)


Fix error message here (indentation and '+')



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


I would simply start from position 0 and find the first unset bit. We won't 
have a lot of containers (O(100) as most). So I won't be too worried about the 
performance here. Add a TODO to do it more efficiently should be sufficient.


- Jie Yu


On Jan. 31, 2016, 5:56 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42587/
> ---
> 
> (Updated Jan. 31, 2016, 5:56 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4345
> https://issues.apache.org/jira/browse/MESOS-4345
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented the `NetClsHandleMgr` class.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
> b4bc52114389d1c1efce2830f4292bd89bb0de7c 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
> ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 
> 
> Diff: https://reviews.apache.org/r/42587/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 42587: Implemented the `NetClsHandleMgr` class.

2016-01-31 Thread Avinash sridharan 

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

(Updated Jan. 31, 2016, 5:56 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Implemented the `NetClsHandleMgr` class.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
b4bc52114389d1c1efce2830f4292bd89bb0de7c 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
ddc1bf0939e5e8995e6f34fe7b8509b51704f63e 

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


Testing
---

make


Thanks,

Avinash sridharan

Re: Review Request 42587: Implemented the `NetClsHandleMgr` class.

2016-01-25 Thread Avinash sridharan 

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

(Updated Jan. 26, 2016, 6:31 a.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Implemented the `NetClsHandleMgr` class.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
7ce5243ed7476a1cfd1371b5ce25b62dc07432db 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
0798de747627ccc45b01a2668e16693757dc69a8 

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


Testing (updated)
---

make


Thanks,

Avinash sridharan