> On June 5, 2015, 6:42 p.m., Vinod Kone wrote: > > I made some minor comments below but I think a better way to do this is to > > *not* write custom masterinfo json <-> protobuf converters. I prefer we > > just add a new optional field (say ipAddress of type string). Then you can > > just leverage the already existing generic protobuf <-> json converters. > > Niklas Nielsen wrote: > We have been working on this for a while now and we are using > JSON::Protobuf() aldready and can enhance it further with your suggestion. > However, the current approach isn't semantically different from the one you > suggest and we would like to move forward and make that change later. Is that > OK with you? > > Marco Massenzio wrote: > Hey Vinod - as Niklas pointed out, we have invested a significant amount > of time on this one, including the manual testing I've done (as summarized on > MESOS-2304) and I'd be really reluctant to start again from scratch. > > As I don't really think there is any semantic difference; my approach > does not introduce any performance penalty (in fact, I believe it may even be > better than a 'generic' one); and, finally, that this does not impact the > readability of the code, we should commit the code 'as is' (with your > suggestions below) and move on. > > There are a ton of features and work to do, and I'd like us to focus on > important issues.
Marco. I appreciate that you invested significant effort to making sure the backwards compatibility story is airtight, but I urge you to spend some extra cycles to simplify the code and avoid tech debt. I honestly don't think it would take too much time to simplify this. I would really like to understand what's the most time consuming part here? The compatibility testing? Can you automate it with niklas's compatibility script? As an aside, going through earlier reviews I saw that BenH made similar comments but it got sidetracked with deprecating "ip". Also, my fault for not jumping on this sooner, but in my defense, I wasn't marked as a reviewer. I would be happy to shepherd this change for you going forward. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/#review86819 ----------------------------------------------------------- On June 3, 2015, 9 p.m., Marco Massenzio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34687/ > ----------------------------------------------------------- > > (Updated June 3, 2015, 9 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 > >
