> On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote:
> > src/Makefile.am, line 1387
> > <https://reviews.apache.org/r/34687/diff/1/?file=972328#file972328line1387>
> >
> >     Is the alignment right here? Try to set your tabstop to 8 :)
> >     Also, shouldn't the ordering bring this further down?

no, it's utterly wrong - I planned to fix it later using vim (Eclipse is all 
off with tabs) but forgot.


> On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote:
> > src/common/protobuf_utils.cpp, line 169
> > <https://reviews.apache.org/r/34687/diff/1/?file=972330#file972330line169>
> >
> >     We have traditionally used stringify(...) if we needed type T to string 
> > conversions. Have you checked if we have one for net::IP?

haven't come across it, and, trust me, I've looked for better ways of getting a 
string out of a net::IP object
would it be in the net::IP class, or tucked somewhere else safe?


> On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote:
> > src/common/protobuf_utils.cpp, lines 202-203
> > <https://reviews.apache.org/r/34687/diff/1/?file=972330#file972330line202>
> >
> >     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));

cool!
will do, thanks.


> On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote:
> > src/tests/common/protobuf_utils_tests.cpp, line 23
> > <https://reviews.apache.org/r/34687/diff/1/?file=972331#file972331line23>
> >
> >     This (and standard c includes) goes first

haven't we changed this recently?
(I was assuming that the protobuf_utils.hpp was the 'relevant' header?)


> On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote:
> > src/tests/common/protobuf_utils_tests.cpp, line 61
> > <https://reviews.apache.org/r/34687/diff/1/?file=972331#file972331line61>
> >
> >     Should we encode this in human readable form?

we probably could, but (a) the code would be pretty gnarly and (b) what's the 
point, really?
this is what that IP string gets turned into when it becomes an int32 - we want 
to make sure this invariant does not get broken by future changes/refactorings.


> On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote:
> > src/tests/common/protobuf_utils_tests.cpp, line 148
> > <https://reviews.apache.org/r/34687/diff/1/?file=972331#file972331line148>
> >
> >     What does 'FullCycle' mean?
> >     Mind added test descriptions as preceeding comments to the test blocks?

eh eh I honestly spent five minutes trying to come up with something 
meaningful: epic fail, clearly!
it actually means we go from UPID to MasterInfo to JSON and back to MasterInfo, 
and we want to get the same thing we started from.

I thought the comment made that a bit clearer, but will elaborate on the point.


- Marco


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


On May 26, 2015, 11: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, 11: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
> 
>

Reply via email to