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