You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2017/06/02 00:33:25 UTC
Re: Review Request 59688: Moved `net::IPNetwork` to
`net::IP::Network`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59688/#review176713
-----------------------------------------------------------
This needs to get split between Stout and Mesos please!
3rdparty/stout/include/stout/ip.hpp
Lines 208 (patched)
<https://reviews.apache.org/r/59688/#comment250144>
Ideally we move this to net::inet::IP::Network::LOOPBACK(), but perhaps add a TODO for now if this is not trivial.
3rdparty/stout/include/stout/ip.hpp
Lines 211 (patched)
<https://reviews.apache.org/r/59688/#comment250145>
Same comment as above for `LOOPBACK_V4()`.
3rdparty/stout/include/stout/ip.hpp
Lines 251 (patched)
<https://reviews.apache.org/r/59688/#comment250147>
Looks like the formatting got funky in this function when you moved, can you please fix? Thanks!
3rdparty/stout/include/stout/ip.hpp
Lines 291-292 (patched)
<https://reviews.apache.org/r/59688/#comment250148>
It would be great to have a comment here explaining why you need to use `std::unique_ptr` so others don't have to try and read your mind! ;-) I don't even always remember the C++ rules here.
Another thought is to use a `std::shared_ptr` here instead. The advantage being that maybe then you wouldn't need the extra copy constructor? This class is constant, i.e., you never manipulate `address_` or `netmask_`, but actually making this `std::shared_ptr<const IP>` might be just as tricky. Either way, comment on why this has to be a pointer!
- Benjamin Hindman
On May 31, 2017, 6:37 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59688/
> -----------------------------------------------------------
>
> (Updated May 31, 2017, 6:37 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Jie Yu.
>
>
> Bugs: MESOS-7488
> https://issues.apache.org/jira/browse/MESOS-7488
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Moved `net::IPNetwork` to `net::IP::Network`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/ip.hpp ad2bd922158eecd12b51b272d2a003b8ec8d3550
> 3rdparty/stout/tests/ip_tests.cpp 4c6f81bd53413360182b447ddb149a18e406870e
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 3aecd8c5a2eb1319fe31ebeba815b6766e50904f
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp ce795628f19f0e8138ad9a0b50b7d644b9d734a8
> src/tests/containerizer/cni_isolator_tests.cpp 565e58ae75e918453e4386f5e35a5a844a8b15f8
> src/tests/utils.hpp a2ae43c6a20ab3753a3501d8ce23ffb0f6ac0005
> src/tests/utils.cpp 053976d3c4cf120ff5e7dff6e96c6862a5b617b1
>
>
> Diff: https://reviews.apache.org/r/59688/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 59688: Moved `net::IPNetwork` to
`net::IP::Network`.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On June 2, 2017, 12:33 a.m., Benjamin Hindman wrote:
> > This needs to get split between Stout and Mesos please!
Will split the patches.
> On June 2, 2017, 12:33 a.m., Benjamin Hindman wrote:
> > 3rdparty/stout/include/stout/ip.hpp
> > Lines 291-292 (patched)
> > <https://reviews.apache.org/r/59688/diff/1/?file=1735933#file1735933line292>
> >
> > It would be great to have a comment here explaining why you need to use `std::unique_ptr` so others don't have to try and read your mind! ;-) I don't even always remember the C++ rules here.
> >
> > Another thought is to use a `std::shared_ptr` here instead. The advantage being that maybe then you wouldn't need the extra copy constructor? This class is constant, i.e., you never manipulate `address_` or `netmask_`, but actually making this `std::shared_ptr<const IP>` might be just as tricky. Either way, comment on why this has to be a pointer!
Valid point. Will add the comment.
About `std::unique_ptr` vs `std::shared_ptr`, the argument is valid (not requiring the copy constructor) and I was thinking about it as well. The problem I had was that semantically it seemed incorrect (in my opnion). Each `IP::Network` is an individual object that does not share state with another copy so thought `std::unique_ptr` better exemplifies this semantic. But I also agree that this is an implementation detail and the object itself is a ready only (we do not allow manipulation of the `address_` and `netmask_` properties). Will defer to your judgement on the use of either.
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59688/#review176713
-----------------------------------------------------------
On May 31, 2017, 6:37 p.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59688/
> -----------------------------------------------------------
>
> (Updated May 31, 2017, 6:37 p.m.)
>
>
> Review request for mesos, Benjamin Hindman and Jie Yu.
>
>
> Bugs: MESOS-7488
> https://issues.apache.org/jira/browse/MESOS-7488
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Moved `net::IPNetwork` to `net::IP::Network`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/ip.hpp ad2bd922158eecd12b51b272d2a003b8ec8d3550
> 3rdparty/stout/tests/ip_tests.cpp 4c6f81bd53413360182b447ddb149a18e406870e
> src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 3aecd8c5a2eb1319fe31ebeba815b6766e50904f
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp ce795628f19f0e8138ad9a0b50b7d644b9d734a8
> src/tests/containerizer/cni_isolator_tests.cpp 565e58ae75e918453e4386f5e35a5a844a8b15f8
> src/tests/utils.hpp a2ae43c6a20ab3753a3501d8ce23ffb0f6ac0005
> src/tests/utils.cpp 053976d3c4cf120ff5e7dff6e96c6862a5b617b1
>
>
> Diff: https://reviews.apache.org/r/59688/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Avinash sridharan
>
>
Re: Review Request 59688: Moved `net::IPNetwork` to `net::IP:Network`.
Posted by Avinash sridharan <av...@mesosphere.io>.
> On June 2, 2017, 12:33 a.m., Benjamin Hindman wrote:
> > 3rdparty/stout/include/stout/ip.hpp
> > Lines 208 (patched)
> > <https://reviews.apache.org/r/59688/diff/1/?file=1735933#file1735933line209>
> >
> > Ideally we move this to net::inet::IP::Network::LOOPBACK(), but perhaps add a TODO for now if this is not trivial.
Have added a TODO for now, but will create a separate patch for `net::inet::IP::Network` once we have the `inet::IP` patch done.
> On June 2, 2017, 12:33 a.m., Benjamin Hindman wrote:
> > 3rdparty/stout/include/stout/ip.hpp
> > Lines 291-292 (patched)
> > <https://reviews.apache.org/r/59688/diff/1/?file=1735933#file1735933line292>
> >
> > It would be great to have a comment here explaining why you need to use `std::unique_ptr` so others don't have to try and read your mind! ;-) I don't even always remember the C++ rules here.
> >
> > Another thought is to use a `std::shared_ptr` here instead. The advantage being that maybe then you wouldn't need the extra copy constructor? This class is constant, i.e., you never manipulate `address_` or `netmask_`, but actually making this `std::shared_ptr<const IP>` might be just as tricky. Either way, comment on why this has to be a pointer!
>
> Avinash sridharan wrote:
> Valid point. Will add the comment.
>
> About `std::unique_ptr` vs `std::shared_ptr`, the argument is valid (not requiring the copy constructor) and I was thinking about it as well. The problem I had was that semantically it seemed incorrect (in my opnion). Each `IP::Network` is an individual object that does not share state with another copy so thought `std::unique_ptr` better exemplifies this semantic. But I also agree that this is an implementation detail and the object itself is a ready only (we do not allow manipulation of the `address_` and `netmask_` properties). Will defer to your judgement on the use of either.
Added the comment explaining why need smart pointers. Please re-open this if you still want to use `shared_ptr` instead of `unique_ptr`.
- Avinash
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59688/#review176713
-----------------------------------------------------------
On June 2, 2017, 2:23 a.m., Avinash sridharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59688/
> -----------------------------------------------------------
>
> (Updated June 2, 2017, 2:23 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Jie Yu.
>
>
> Bugs: MESOS-7488
> https://issues.apache.org/jira/browse/MESOS-7488
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Moved `net::IPNetwork` to `net::IP:Network`.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/ip.hpp ad2bd922158eecd12b51b272d2a003b8ec8d3550
> 3rdparty/stout/tests/ip_tests.cpp 4c6f81bd53413360182b447ddb149a18e406870e
>
>
> Diff: https://reviews.apache.org/r/59688/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Avinash sridharan
>
>