> On May 13, 2014, 12:58 a.m., Vinod Kone wrote: > > src/linux/routing/filter/icmp.hpp, lines 77-92 > > <https://reviews.apache.org/r/20295/diff/4/?file=569527#file569527line77> > > > > Why not take action::Action and use the dynamic cast trick in an > > earlier review? > > Jie Yu wrote: > Yeah, this was a deliberate design (as you saw in ARP and IP filters) > that we don't expose an interface that we don't need. For example, in the > current network isolator, we never add a IP filter that has mirror action, as > a result, we don't support adding IP filter that has a mirror action > (although it will be very trivial to add given the internal APIs). > > The same philosophy applies for "Classifiers". In theory, we could > support src/dest MACs, IPs and ports. But we choose to support only a subset > of that that we need in the network isolator. > > But since this is a library, I think it's fine to expose all of that even > if we don't use (we may want to use in the future). So I am gonna do the > change you suggested. > > Now that we want to be generic, I think the current Classifier > construction interface is not quite good and is not quite extensible. > Especially, using the constructors and relying on the order specified by the > user is not quite reliable. For example, to create an IP classifier: > > ip::Classifier(mac, ip, ports1, ports2) > > The user may wonder which one is source ports and which one is > destination ports. If we add more fields in the future, it's gonna be > problematic because we need to check all the use sites to make sure the order > is correct after the change. > > Here is what I propose for the new Classifier interface: > > ip::Classifier() > .setDestinationMAC(mac) > .setDestinationIP(ip) > .setSourcePorts(ports1) > .setDestinationPorts(ports2); > > If we want to introduce more fields in the future, we can simply add > another 'set' function and no change to the current use sites. > > What do you think? > > > >
Discussed with Vinod offline. We decided to go with the existing interface for now. - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20295/#review42794 ----------------------------------------------------------- On April 28, 2014, 5:02 p.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20295/ > ----------------------------------------------------------- > > (Updated April 28, 2014, 5:02 p.m.) > > > Review request for mesos, Benjamin Hindman, Chi Zhang, Ian Downes, Vinod > Kone, and Cong Wang. > > > Bugs: MESOS-1228 > https://issues.apache.org/jira/browse/MESOS-1228 > > > Repository: mesos-git > > > Description > ------- > > This is Part 3 of the linux routing library. > > > Diffs > ----- > > src/Makefile.am d2e006d > src/linux/routing/filter/icmp.hpp PRE-CREATION > src/linux/routing/filter/icmp.cpp PRE-CREATION > src/tests/routing_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/20295/diff/ > > > Testing > ------- > > make check > sudo make check > > > Thanks, > > Jie Yu > >
