-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34687/#review86176
-----------------------------------------------------------


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.


src/common/http.hpp
<https://reviews.apache.org/r/34687/#comment138117>

    Please end sentences with a period.



src/common/http.cpp
<https://reviews.apache.org/r/34687/#comment138109>

    Please end sentences with a period. Please do a sweep of your review, I see 
this a handful of places. Looks like a great candidate for a clang-tidy check. 
;-)



src/common/http.cpp
<https://reviews.apache.org/r/34687/#comment138115>

    Please use identical names in the declaration and definition: 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html?showone=Function_Declarations_and_Definitions#Function_Declarations_and_Definitions
    
    There will be a clang-tidy check that we can turn on for this soon, would 
love help with that: 
https://github.com/llvm-mirror/clang-tools-extra/blob/master/clang-tidy/readability/NamedParameterCheck.h



src/common/http.cpp
<https://reviews.apache.org/r/34687/#comment138112>

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



src/common/http.cpp
<https://reviews.apache.org/r/34687/#comment138114>

    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.



src/common/http.cpp
<https://reviews.apache.org/r/34687/#comment138113>

    Why the extra temporary? Doing the compilers job Marco? ;-)
    
    net::IP ip(ntohl(ipAsInt.get().value));



src/common/parse.hpp
<https://reviews.apache.org/r/34687/#comment138106>

    Is this a rouge 'template<>' that shouldn't be there?



src/common/parse.hpp
<https://reviews.apache.org/r/34687/#comment138108>

    Operators, including '+' on previous line please.



src/tests/common/parse_tests.cpp
<https://reviews.apache.org/r/34687/#comment138123>

    ASSERT_ERROR



src/tests/common/parse_tests.cpp
<https://reviews.apache.org/r/34687/#comment138122>

    ASSERT_ERROR


- Benjamin Hindman


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