mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Niklas Nielsen" <...@qni.dk>
Subject Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON
Date Wed, 27 May 2015 02:18:05 GMT

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


Hey Marco,

First pass: One high-level question to your patch: Could we reuse JSON::Protobuf() to some
of the modeling? https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp#L568

If the IP is wrong type, we can perhaps correct it after parsing it with JSON::Protobuf()?


src/Makefile.am
<https://reviews.apache.org/r/34687/#comment136859>

    Is the alignment right here? Try to set your tabstop to 8 :)
    Also, shouldn't the ordering bring this further down?



src/common/protobuf_utils.hpp
<https://reviews.apache.org/r/34687/#comment136869>

    Shouldn't parse() go in https://github.com/apache/mesos/blob/master/src/common/parse.hpp?



src/common/protobuf_utils.hpp
<https://reviews.apache.org/r/34687/#comment136868>

    We traditionally spell out variable names. Here, minfo would be masterInfo or simply 'info'



src/common/protobuf_utils.cpp
<https://reviews.apache.org/r/34687/#comment136867>

    We have traditionally used stringify(...) if we needed type T to string conversions. Have
you checked if we have one for net::IP?



src/common/protobuf_utils.cpp
<https://reviews.apache.org/r/34687/#comment136866>

    Two newlines



src/common/protobuf_utils.cpp
<https://reviews.apache.org/r/34687/#comment136865>

    Could we do this error message inline in the return Error(...)? We can use stringify()
to get the JSON string:
    
    return Error("Missing ..." + stringify(json));



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

    This (and standard c includes) goes first



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

    We usually spell out variable names. MasterInfo info or MasterInfo masterInfo, would be
the to-go names here :)



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

    Should we encode this in human readable form?



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

    This can be made const, right?



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

    Let's move this closer to where we use it (line 141)



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

    What does 'FullCycle' mean?
    Mind added test descriptions as preceeding comments to the test blocks?


- Niklas Nielsen


On May 26, 2015, 4:54 p.m., Marco Massenzio wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> -----------------------------------------------------------
> 
> (Updated May 26, 2015, 4:54 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 814468e3c5c750a6649b5eeb7c7f945f9e025c19 
>   src/common/protobuf_utils.hpp 9ecd2343689252af1b997392ec367d14d55ac7d1 
>   src/common/protobuf_utils.cpp bd6996159e73bf63bb7c2fa3a28def6a2be92b1b 
>   src/tests/common/protobuf_utils_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