> On May 27, 2015, 2:18 a.m., Niklas Nielsen wrote:
> > src/common/protobuf_utils.hpp, line 83
> > <https://reviews.apache.org/r/34687/diff/1/?file=972329#file972329line83>
> >
> >     Shouldn't parse() go in 
> > https://github.com/apache/mesos/blob/master/src/common/parse.hpp?
> 
> Marco Massenzio wrote:
>     I don't really know - in the file there's nothing to say one way or the 
> other: it just seems a bunch of `parse()` methods (in the `flagss::` 
> namespace) converting from `string` to `protobuf`?
>     I guess so, if one takes the stance the similarly named methods should 
> all be in the same header file...
>     
>     I actually prefer to keep methods *logically* grouped and, ideally, the 
> two to/from conversions next to each other, so it's easier to keep them in 
> sync if either formats (JSON / PBuf) ought to change.
>     
>     Also, not particularly fond of methods kept inline and in header files - 
> but that's a matter of taste, I can easily be convinced either way.
> 
> Niklas Nielsen wrote:
>     We have been catious on landing too many helpers in protobuf_utils in the 
> past.
>     If you want to do a logical separation, should it go in a 
> protobuf_parse.hpp instead (taken that it is a pattern we will see more 
> often)?
>     I still like the idea to land it in parse.hpp, as it may be Foo -> Bar 
> convertions (is 'parse' the right term, taken that we already have a JSON 
> object? Or is 'convert' more precise?)

well, probably *too* cautious :)
that file has only other *three* methods, and is, overall, including my 
changes, approx 200 lines

If you really feel strongly that we should create yet another file, sure, why 
not - but it seems to me it does not add any value (and it's not like we have 
20 helpers there: there are now like, 5-6, including the two I just created).

I'm not trying to be difficult here, Nik - but really, to create a file to add 
a single method, is it really going to make things better?


- Marco


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


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