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


Looking much better! :)

A few suggestions below and let's get this in.


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

    We haven't established a doxygen style in the style guide yet, but 
shouldn't we expand this to:
    
    /**
     *  Foo Bar Baz
     */
    
    ?



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

    s/`/'/
    Error messages don't end with period (see 
http://mesos.apache.org/documentation/latest/mesos-c++-style-guide/)



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

    Move this to 'newline' - see: 
https://github.com/apache/mesos/blob/master/3rdparty/libprocess/include/process/clock.hpp#L28



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

    s/`/'/ and end with period



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

    What if in() is an error? Shouldn't we check for that too?



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

    Mind adding a test description?



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

    Do you want ASSERT_SOME(ip.in());?



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

    As jsonMasterInfo will be a json object, how about doing equality between 
json objects instead of stringifying?
    
    auto expected = JSON::parse("{ hostname ...");
    
    ASSERT_EQ(expected.get(), jsonMasterInfo.get());
    
    That would be robust to any spaces,newlines in the inlined JSON.
    
    If you like it, we can do it here and below.



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

    Test description :)



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

    End with period?



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

    s/the/The/



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

    End with period.



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

    Need an ASSERT_SOME() on jsonMasterInfo.get().find<JSON::String>("id") ?



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

    Shouldn't this include be located near line 35?



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

    Test description :)



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

    s/yields back/yields/?



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

    MInfo? Should it be jsonMasterInfo?



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

    Just tried this out: include <mesos/type_utils.hpp> and you should have the 
== operator overload, be able to strip the '.SerializeAsString()' and kill the 
comment :)


- Niklas Nielsen


On June 2, 2015, 2:52 p.m., Marco Massenzio wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> -----------------------------------------------------------
> 
> (Updated June 2, 2015, 2:52 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 f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
>   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