mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benjamin Hindman" <b...@berkeley.edu>
Subject Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON
Date Wed, 10 Jun 2015 08:12:32 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message