----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/#review86453 -----------------------------------------------------------
Looking much better! :) A few suggestions below and let's get this in. src/common/http.cpp <https://reviews.apache.org/r/34687/#comment138457> We haven't established a doxygen style in the style guide yet, but shouldn't we expand this to: /** * Foo Bar Baz */ ? src/common/http.cpp <https://reviews.apache.org/r/34687/#comment138467> s/`/'/ Error messages don't end with period (see http://mesos.apache.org/documentation/latest/mesos-c++-style-guide/) src/common/parse.hpp <https://reviews.apache.org/r/34687/#comment138460> Move this to 'newline' - see: https://github.com/apache/mesos/blob/master/3rdparty/libprocess/include/process/clock.hpp#L28 src/common/parse.hpp <https://reviews.apache.org/r/34687/#comment138468> s/`/'/ and end with period src/common/parse.hpp <https://reviews.apache.org/r/34687/#comment138470> What if in() is an error? Shouldn't we check for that too? src/tests/common/http_tests.cpp <https://reviews.apache.org/r/34687/#comment138472> Mind adding a test description? src/tests/common/http_tests.cpp <https://reviews.apache.org/r/34687/#comment138473> Do you want ASSERT_SOME(ip.in());? src/tests/common/http_tests.cpp <https://reviews.apache.org/r/34687/#comment138475> As jsonMasterInfo will be a json object, how about doing equality between json objects instead of stringifying? auto expected = JSON::parse("{ hostname ..."); ASSERT_EQ(expected.get(), jsonMasterInfo.get()); That would be robust to any spaces,newlines in the inlined JSON. If you like it, we can do it here and below. src/tests/common/http_tests.cpp <https://reviews.apache.org/r/34687/#comment138476> Test description :) src/tests/common/http_tests.cpp <https://reviews.apache.org/r/34687/#comment138477> End with period? src/tests/common/http_tests.cpp <https://reviews.apache.org/r/34687/#comment138479> s/the/The/ src/tests/common/http_tests.cpp <https://reviews.apache.org/r/34687/#comment138478> End with period. src/tests/common/http_tests.cpp <https://reviews.apache.org/r/34687/#comment138480> Need an ASSERT_SOME() on jsonMasterInfo.get().find<JSON::String>("id") ? src/tests/common/parse_tests.cpp <https://reviews.apache.org/r/34687/#comment138481> Shouldn't this include be located near line 35? src/tests/common/parse_tests.cpp <https://reviews.apache.org/r/34687/#comment138482> Test description :) src/tests/common/parse_tests.cpp <https://reviews.apache.org/r/34687/#comment138463> s/yields back/yields/? src/tests/common/parse_tests.cpp <https://reviews.apache.org/r/34687/#comment138464> MInfo? Should it be jsonMasterInfo? src/tests/common/parse_tests.cpp <https://reviews.apache.org/r/34687/#comment138487> Just tried this out: include <mesos/type_utils.hpp> and you should have the == operator overload, be able to strip the '.SerializeAsString()' and kill the comment :) - Niklas Nielsen On June 2, 2015, 2:52 p.m., Marco Massenzio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34687/ > ----------------------------------------------------------- > > (Updated June 2, 2015, 2:52 p.m.) > > > Review request for mesos, haosdent huang and Niklas Nielsen. > > > Bugs: MESOS-2340 > https://issues.apache.org/jira/browse/MESOS-2340 > > > Repository: mesos > > > Description > ------- > > Jira: MESOS-2340 > > This is a preliminary step to enabling JSON API > for Master Discovery via Zookeeper. > > We currently save the MasterInfo PB to ZK > serializing directly the binary data, so that > for clients to retrieve that information, they > need to either link up with libmesos or > obtain the PB definition (in mesos/mesos.proto). > > This change only provides the (de)serialization > utility methods and associated tests. > > > Diffs > ----- > > src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a > src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 > src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a > src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 > src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 > src/tests/common/parse_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/34687/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Marco Massenzio > >
