----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29290/#review75530 -----------------------------------------------------------
Looking great! The code is much more cleaner now! src/common/protobuf_utils.hpp <https://reviews.apache.org/r/29290/#comment122692> No need to introduce this helper for now since the implementation is just one line right now. Let's re-consider adding some helpers if Mesos starts to support ipv6 (i.e., store ipv6 in master info). src/common/protobuf_utils.cpp <https://reviews.apache.org/r/29290/#comment122693> Kill this one. src/common/protobuf_utils.cpp <https://reviews.apache.org/r/29290/#comment122695> Let add this TODO later once we start to add ipv6 impl. src/common/protobuf_utils.cpp <https://reviews.apache.org/r/29290/#comment122694> Please add a NOTE about the fact that we store ip in network order (which is not standard and point to the ticket MESOS-1201). src/linux/routing/diagnosis/diagnosis.cpp <https://reviews.apache.org/r/29290/#comment122697> Why returns a Try here? src/linux/routing/filter/icmp.cpp <https://reviews.apache.org/r/29290/#comment122698> `struct in_addr` please src/linux/routing/filter/icmp.cpp <https://reviews.apache.org/r/29290/#comment122703> Please add a NOTE here saying that filters only support IPv4 currently. src/linux/routing/filter/icmp.cpp <https://reviews.apache.org/r/29290/#comment122705> ``` return Error("Destination IP is not an IPv4 address"); ``` src/linux/routing/filter/icmp.cpp <https://reviews.apache.org/r/29290/#comment122706> Opps. You've already done `value = ntohl(value)` above, why do you need to do it again here? src/linux/routing/filter/ip.cpp <https://reviews.apache.org/r/29290/#comment122707> Ditto. Please add a NOTE here. src/linux/routing/filter/ip.cpp <https://reviews.apache.org/r/29290/#comment122709> I don't think you need ntohl here. src/linux/routing/route.cpp <https://reviews.apache.org/r/29290/#comment122696> Adjust the comments. Get destination IP network src/master/master.cpp <https://reviews.apache.org/r/29290/#comment122711> Ditto. Remove this TODO and add a NOTE about the use of network order for ip. src/slave/containerizer/isolators/network/port_mapping.hpp <https://reviews.apache.org/r/29290/#comment122712> s/hostIP/hostIPNetwork src/slave/containerizer/isolators/network/port_mapping.cpp <https://reviews.apache.org/r/29290/#comment122713> Kill this TODO and add a NOTE somewhere saying that this isolator is for IPv4 only. src/tests/master_contender_detector_tests.cpp <https://reviews.apache.org/r/29290/#comment122716> I guess they just need a random IP. Please just do: ``` master.address.ip = net::IP::parse("1.2.3.4").get(); ``` Here and every other cases below. src/tests/master_tests.cpp <https://reviews.apache.org/r/29290/#comment122668> Let's do this: ``` EXPECT_EQ( master.get().address.ip, net::IP(ntohl(masterInfo.get().ip()))); ``` src/tests/master_tests.cpp <https://reviews.apache.org/r/29290/#comment122669> Ditto here. src/tests/routing_tests.cpp <https://reviews.apache.org/r/29290/#comment122715> Just do ``` net::IP ip = net::IP::parse("1.2.3.4").get(); ``` Here and other cases below! - Jie Yu On March 6, 2015, 8:02 p.m., Evelina Dumitrescu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29290/ > ----------------------------------------------------------- > > (Updated March 6, 2015, 8:02 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 > ------- > > see summary > > > Diffs > ----- > > src/common/protobuf_utils.hpp a5793918a2c1bc1c13432653c4219de7283fefd1 > src/common/protobuf_utils.cpp f57213a5ab33787296d17abccfca71345a90d9a4 > src/linux/routing/diagnosis/diagnosis.cpp > 136ba379efbbe4200c0e9f794a2966ffee174fff > src/linux/routing/filter/icmp.hpp 7b478c4e23a2fb0895021ce5fb30bdf78131b871 > src/linux/routing/filter/icmp.cpp 86bd67b71a590b88344adbe10fd1b44ea1b5148d > src/linux/routing/filter/ip.hpp 932ed4bbf57e60261ff3b48ae242a7dd5e1f4260 > src/linux/routing/filter/ip.cpp 922a732c3543a072674208b123fdfadbef2b15f2 > src/linux/routing/route.hpp 9e5339192aeb5e26b932a54e7b9110430eafbd9b > src/linux/routing/route.cpp b0eda7b662eca0ba1357e558f6f6b1474067b06d > src/master/http.cpp b8eef69505b147d4c8a0e005dff545b9fc12a220 > src/master/master.cpp 68ca19a9ae680e3ae5bd433a9842baf69f2360ec > src/sched/sched.cpp 0f857032a4b38a73f2dcd7e069e9f97a0941847b > src/scheduler/scheduler.cpp 5ae27960c6a4e05ce48eb9d8c8f3390183e74f46 > src/slave/containerizer/isolators/network/port_mapping.hpp > 8443097b2c79fef5ae0e23a3fb815ffec0318a93 > src/slave/containerizer/isolators/network/port_mapping.cpp > 5227987cdb7b904c2f4bb2bdf5c5d705a435010d > src/slave/slave.cpp 2d52ea0c440fd530174b0e44a59c8ae68fa2616c > src/tests/master_contender_detector_tests.cpp > f8c7f2cf81aa8376ab0da545270406300a385ba6 > src/tests/master_tests.cpp 580e1f818201f951c11e4e652a7941fcd888356d > src/tests/port_mapping_tests.cpp e2c8ba12b5574b06a6ba60c3c3a30b63cea1d23c > src/tests/routing_tests.cpp 3cda6ab8c1ad24e4b7d0b9aeda2abc595fc857a5 > > Diff: https://reviews.apache.org/r/29290/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Evelina Dumitrescu > >
