> On June 2, 2015, 9:10 a.m., Benjamin Hindman wrote: > > I haven't done a full review here but took interest in this because the > > required 'ip' field of the MasterInfo protobuf (as well as other protobufs) > > is deprecated because it doesn't support IPv6. We need to make 'ip' an > > optional field and replace it by an 'address' field which includes both the > > 'hostname' or 'ip' and the port that can be parsed by our Address. Once we > > do that, then we shoudn't need any special JSON conversion or parsing.
Well, according to Protobuf Law, you can't: `required` is like a diamond: it's for life! ;) But I'm all for it - if you think this is safe to do over a couple of releases? I had already created https://issues.apache.org/jira/browse/MESOS-2736 - please add your suggestions/comments there. Once we finalize that, I think patching this code to make it work with the new design should be trivial. > On June 2, 2015, 9:10 a.m., Benjamin Hindman wrote: > > src/common/http.hpp, line 52 > > <https://reviews.apache.org/r/34687/diff/4/?file=975083#file975083line52> > > > > Please end sentences with a period. Just so you know, in my spare time I'm submitting patches to cpplint.py :-) (thanks for catching it!) > On June 2, 2015, 9:10 a.m., Benjamin Hindman wrote: > > src/common/http.cpp, lines 212-213 > > <https://reviews.apache.org/r/34687/diff/4/?file=975084#file975084line212> > > > > Please see formatting for braces: > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Braced_Initializer_List_Format > > > > tl;dr; Use braced-initializer list construction like a constructor. > > > > JSON::Protobuf protobuf{masterInfo}; > > > > Please fix throughout this review. > > > > But this brings me to a second question, why use braced-initializer > > list here at all when you're just calling a normal constructor (i.e., not a > > constructor that takes an std::initializer_list or an implicit constructor > > that just sets the fields of the object). > > > > Finally, this can be simplified: > > > > JSON::Object json(JSON::Protobuf(masterInfo)); sorry - this was a remnant from some trial code I'd run when playing around with JSON::Protobuf (just to see what came out). Fixed. I'm hoping I can bring cpplint to detect all this stuff. > On June 2, 2015, 9:10 a.m., Benjamin Hindman wrote: > > src/common/http.cpp, line 219 > > <https://reviews.apache.org/r/34687/diff/4/?file=975084#file975084line219> > > > > This seems rather aggressive, while I understand that 'ip' is required, > > that doesn't mean that someone might not pass an incomplete protobuf where > > this could crash hard instead of just returning an Error. Well, I'd asked around whether there was a method to check both !error && !none and CHECK_SOME() was what had come up. My bad. > On June 2, 2015, 9:10 a.m., Benjamin Hindman wrote: > > src/common/http.cpp, lines 220-225 > > <https://reviews.apache.org/r/34687/diff/4/?file=975084#file975084line220> > > > > Why the extra temporary? Doing the compilers job Marco? ;-) > > > > net::IP ip(ntohl(ipAsInt.get().value)); old habit - when I see gnarly code, my brain immediately projects an image of me doing debugging :) hey, compared to a manager's job, a compiler's is a piece of cake! - Marco ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/#review86176 ----------------------------------------------------------- On May 31, 2015, 2:58 a.m., Marco Massenzio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34687/ > ----------------------------------------------------------- > > (Updated May 31, 2015, 2:58 a.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 e7281ac6667562a27b0427b0ba7f41de98702928 > 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 > >
