mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Evelina Dumitrescu" <evelina.a.dumitre...@gmail.com>
Subject Re: Review Request 29288: stout: Created IP address abstraction for different protocol families
Date Thu, 19 Feb 2015 16:50:04 GMT


> On Feb. 18, 2015, 1:28 a.m., Jie Yu wrote:
> > Glad to see this being worked on, Thanks for all the effort!
> > 
> > This is a partial review. Let's address the following comments first and then I'll
take a second pass.
> > 
> > One high level comment is that we should separate this giant patch. Many changes
are being mixed together, making it very hard for reviewers to digest. For example, you renamed
`ip` to `IP::fromLinkDevice`. I think that can be pulled into a separate patch, right?
> > 
> > Also, let's try not to introduce helper functions that are not currently being used.
You can leave a TODO there. That can also bring this patch smaller. For example, you don't
have to add the implementation for IPv6 case in `IP::fromLinkDevice` because it's not being
used yet. You can always leave a TODO and fill the implementation later in a separate patch.
That'll make the reviewer's life much better.
> > 
> > I would suggest we first refactor `net::IP` and separate the concepts of IP address
and IP network (see my comments below). No need to introduce the concept of family (and all
IPv6 stuffs) yet in this patch. You need to update all the callsites that use `net::IP` (e.g.,
the port mapping isolator might want to take `net::IPNetwork` instead of `net::IP` in some
cases). Let's start with this first!
> > 
> > And then, in the second patch, you can make `net::IP` IPv6 capable. That means you
need to store a union. Simply add TODO in those helper functions that you think you need to
fill IPv6 implementaion in. Let's try to keep that patch small as well.
> > 
> > Then, we'll see what we go from there. Does that sound good to you?

I appreciate a lot that you took a look at my patches!

If we splitt the patch into IP and IPNetwork, then the netmask from the IPNetwork class should
be optional.
A situation where we need the net mask as optional is fromLinkDevice.
We will end up with the IP and IPNetwork classes representing the same thing.

I agree with splitting this patch and introduce the changes step by step.


- Evelina


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


On Feb. 12, 2015, 8:05 p.m., Evelina Dumitrescu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29288/
> -----------------------------------------------------------
> 
> (Updated Feb. 12, 2015, 8:05 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Dominic Hamon, Jie Yu, Joris Van Remoortere,
and Niklas Nielsen.
> 
> 
> Bugs: MESOS-1919
>     https://issues.apache.org/jira/browse/MESOS-1919
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Created the inner class InAddrStorage encapsulated inside the IP class.
> The class uses a union with the in_addr and in6_addr fields.
> I considered that the The MasterInfo protobuffers should have both an ipv4 and an ipv6
field.
> I intend to use the same Classifiers, addition, removal and update of container filters,
but write different encode/decode functions for IPv4/ICMP and IPv6/ICMPv6 because the processing
of the protocol headers differ.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/net.hpp a0210ea6440086246aafe632f86498abbb70719a

>   3rdparty/libprocess/3rdparty/stout/tests/net_tests.cpp 425132e5d7c3770be4a5a39feea5a2f22179b871

> 
> Diff: https://reviews.apache.org/r/29288/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Evelina Dumitrescu
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message