> 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. > > Vinod Kone wrote: > 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. > > Marco Massenzio wrote: > The upgrade/change of MasterInfo is tracked in > https://issues.apache.org/jira/browse/MESOS-2736 > which I believe is a different topic than what is addressed here (which > is tracked here: https://issues.apache.org/jira/browse/MESOS-2807 - I'll > update this patch description in a sec). > > In any event, as `ip` is a required int32 field, we MUST support it, no > matter what - this patch does this, correctly, without introducing "tech > debt" (I don't see where I'm doing that). > > Thanks in advance for your understanding. > > Vinod Kone wrote: > My suggestion is to support current ip field by keeping it asis, i.e., > let it be a number in json string. The reasoning is that, if we end up > deciding that the easiest way to support ipv6 is by having a string field in > the protobuf, then we would've 2 redundant string representations of the ip > address in the resulting json. > > The tech debt part i was referring to was that, now there is are a set of > custom functions for protobuf <-> json conversions for this protobuf which > need to be maintained (while model() is not bad, parse() is doing a lot of > work and seems to have written without the knowledge that there is an > existing generic parse function IIRC?). For example, if someone adds new > fields to MasterInfo they have to come and update these functions. Not to > mention, you have added a bunch of tests to test the custom parsing logic, > which could likely be eliminated if you can reuse the existing generic > functions. > > Feel free to ping me on IRC if you want to discuss further.
Folks, let's leverage what we have here as it's pretty clear from this discussion that we haven't figured out the answer yet for deprecating the 'ip' field and adding an IPv6 supported 'address' field or something similar. It's not worth rewriting this from scratch so instead let's do the next best thing (as we have other places too) and leave some great comments and TODOs around why the current approach was taken and what are the next steps to fix it. I think we're all on the same page that not having to write our own 'model' and 'parse' functions or have the JSON types differ from the protobuf types is the preferred approach, but given we're going to be deprecating the 'ip' field all together we can simply remove it completely (after making it optional) while simultaneously introducing the new field. It'll mean we have to keep the custom 'model' and 'parse' functions around a bit longer (while the 'ip' field still exists), but provided they're clearly documented this should be very minor and manageable. Let's keep the cadence going please! - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34687/#review86819 ----------------------------------------------------------- On June 5, 2015, 8:18 p.m., Marco Massenzio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34687/ > ----------------------------------------------------------- > > (Updated June 5, 2015, 8:18 p.m.) > > > Review request for mesos, haosdent huang and Niklas Nielsen. > > > Bugs: MESOS-2807 > https://issues.apache.org/jira/browse/MESOS-2807 > > > 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 > >
