mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Niklas Nielsen" <...@qni.dk>
Subject Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON
Date Thu, 11 Jun 2015 17:05:01 GMT


> On June 10, 2015, 8:37 p.m., Ben Mahler wrote:
> > How about adding an 'Address' message, which can contain 'ip' and 'port' for now?
> > 
> > ```
> > message Address {
> >   required string ip;
> >   required uint32 port;
> >   
> >   // Later we can add 'hostname' or 'public_hostname', etc!
> > }
> > ```
> > 
> > In the future, we can further use this in other protobufs to have a conistent representation
(e.g. SlaveInfo only has 'hostname' and 'port' currently, no 'ip'!). That way, you can add
an address to MasterInfo:
> > 
> > ```
> > message MasterInfo {
> >   required string id = 1;
> >   required uint32 ip = 2;
> >   required uint32 port = 3 [default = 5050];
> >   optional string pid = 4;
> >   optional string hostname = 5;
> >   
> >   optional Address address = 6;
> > }
> > ```
> > 
> > This way, you don't need all the custom logic introduced here, and consequently
compatibility is easier to test (i.e. we have already tested our JSON <-> Protobuf conversion
facilities).
> > 
> > Have you guys considered this approach?

@BenH, Vinod and BenM: We have too many chefs her. Let's please assign one to drive this.
It sounds like we have to take this back to the design (and JIRA discussions).

Here is my concern; I was assigned as shepherd on this and got blocked in the 11th hour.
There is very little space to iterate on this and the patch would have worked fine. It was
a part of a bigger patch set (zookeeper detection for HTTP, which Vinod drove) and I am amazed
that a conversion between proto to json and vice-versa takes more than 2 weeks back and forth,
to then (most likely) getting dropped. This is one example where both committers and contributors
(us) have a bad experience. I would like to know how to get better and avoid this in the future,
without giving up 'shepherd'ship.

@Marco; I am sorry that we wasted your (and our) time. Let's try to fix the process. It hurts
me as well as I thought we had this in a good shape but it sounds to me like we need to drop
this patch.


- Niklas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34687/#review87520
-----------------------------------------------------------


On June 5, 2015, 1: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, 1: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