> 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
> 
>

Reply via email to