mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marco Massenzio" <ma...@mesosphere.io>
Subject Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON
Date Tue, 02 Jun 2015 21:28:31 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message