----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/#review85311 -----------------------------------------------------------
Hey Marco, First pass: One high-level question to your patch: Could we reuse JSON::Protobuf() to some of the modeling? https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp#L568 If the IP is wrong type, we can perhaps correct it after parsing it with JSON::Protobuf()? src/Makefile.am <https://reviews.apache.org/r/34687/#comment136859> Is the alignment right here? Try to set your tabstop to 8 :) Also, shouldn't the ordering bring this further down? src/common/protobuf_utils.hpp <https://reviews.apache.org/r/34687/#comment136869> Shouldn't parse() go in https://github.com/apache/mesos/blob/master/src/common/parse.hpp? src/common/protobuf_utils.hpp <https://reviews.apache.org/r/34687/#comment136868> We traditionally spell out variable names. Here, minfo would be masterInfo or simply 'info' src/common/protobuf_utils.cpp <https://reviews.apache.org/r/34687/#comment136867> We have traditionally used stringify(...) if we needed type T to string conversions. Have you checked if we have one for net::IP? src/common/protobuf_utils.cpp <https://reviews.apache.org/r/34687/#comment136866> Two newlines src/common/protobuf_utils.cpp <https://reviews.apache.org/r/34687/#comment136865> Could we do this error message inline in the return Error(...)? We can use stringify() to get the JSON string: return Error("Missing ..." + stringify(json)); src/tests/common/protobuf_utils_tests.cpp <https://reviews.apache.org/r/34687/#comment136864> This (and standard c includes) goes first src/tests/common/protobuf_utils_tests.cpp <https://reviews.apache.org/r/34687/#comment136860> We usually spell out variable names. MasterInfo info or MasterInfo masterInfo, would be the to-go names here :) src/tests/common/protobuf_utils_tests.cpp <https://reviews.apache.org/r/34687/#comment136870> Should we encode this in human readable form? src/tests/common/protobuf_utils_tests.cpp <https://reviews.apache.org/r/34687/#comment136863> This can be made const, right? src/tests/common/protobuf_utils_tests.cpp <https://reviews.apache.org/r/34687/#comment136861> Let's move this closer to where we use it (line 141) src/tests/common/protobuf_utils_tests.cpp <https://reviews.apache.org/r/34687/#comment136862> What does 'FullCycle' mean? Mind added test descriptions as preceeding comments to the test blocks? - Niklas Nielsen On May 26, 2015, 4:54 p.m., Marco Massenzio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34687/ > ----------------------------------------------------------- > > (Updated May 26, 2015, 4:54 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 814468e3c5c750a6649b5eeb7c7f945f9e025c19 > src/common/protobuf_utils.hpp 9ecd2343689252af1b997392ec367d14d55ac7d1 > src/common/protobuf_utils.cpp bd6996159e73bf63bb7c2fa3a28def6a2be92b1b > src/tests/common/protobuf_utils_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/34687/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Marco Massenzio > >
