mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marco Massenzio" <ma...@mesosphere.io>
Subject Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON
Date Fri, 05 Jun 2015 19:48:48 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?

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


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


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