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

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.


- Vinod


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

Reply via email to