mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alexander Rukletsov" <ruklet...@gmail.com>
Subject Re: Review Request 36663: Added ip_address field to MasterInfo
Date Thu, 30 Jul 2015 07:56:05 GMT


> On July 29, 2015, 1:01 p.m., Alexander Rukletsov wrote:
> > include/mesos/mesos.proto, line 399
> > <https://reviews.apache.org/r/36663/diff/5/?file=1021578#file1021578line399>
> >
> >     `ip` and `port` are required, while `address` is optional. Is it intentional
/ doesn't it introduce a pitfall?
> 
> Marco Massenzio wrote:
>     You cannot add a `required` field in an existing protobuf, or you break existing
servers (in particular, we would have all <=0.22.x Mesos to break a 0.23 when the latter
does Leader contention/detection)  - that's PB 101 :)
>     
>     Did you mean the fields **inside** `Address` are required, or the **siblings** of
`address`?
>     
>     If the latter, there's nothing you can do about it - as I mentioned, new fields **Must**
be `optional` and you can't mess with existing ones.
>     If the former, no - it's perfectly logically consistent: `address` may or may not
be there; if it is there, then it **must** have `ip` and `port` set.

Sorry for not expressing myself clearly.

1. IIUC, we can introduce `required` fields in the same way we retire them: via optional transisiton.
I am not saying we definitely need to do that here (I haven't though about that long enough),
but it looks technically possible to me.
2. If we forget about technical difficulties for a while, it looks to me that we change a
sematics: what was required becomes optional. Does it make sense to write a comment saying
that the optional `address` field is guaranteed to be there? Does it make sense to introduce
some sort of validation that this field is always present even though it's optional?


- Alexander


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


On July 25, 2015, 12:36 a.m., Marco Massenzio wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36663/
> -----------------------------------------------------------
> 
> (Updated July 25, 2015, 12:36 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-2736
>     https://issues.apache.org/jira/browse/MESOS-2736
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added address field to MasterInfo
>     
>     As part of the new HTTP API and the need to
>     provide a better interface for clients that
>     do not integrate libmesos, we provide the IP
>     address of the Leader Master in the information
>     that gets serialized (in JSON) to ZooKeeper.
>     
>     This will eventually supersede the `ip`, `port`
>     and `hostname` fields that are currently in
>     MasterInfo an which cannot fully support IPv6
>     addresses.
>     
>     Jira: MESOS-2736
>     
>     Review: https://reviews.apache.org/r/36663
> 
> 
> Diffs
> -----
> 
>   CHANGELOG 1a8649d00f3bcc792a75c11dedeb42a667e2ce88 
>   include/mesos/mesos.proto e015c81d5052214ef8207642e23b3892a6123c9a 
>   src/common/protobuf_utils.cpp d900707ae64ad92c0c0ddc2996324b61121c8594 
>   src/master/master.cpp 7796630a93705bd62157e98e1e4855c68ea7cd0a 
>   src/tests/master_contender_detector_tests.cpp d7a3b46b2e437818631064ae34317e49c9aa3748

> 
> Diff: https://reviews.apache.org/r/36663/diff/
> 
> 
> Testing
> -------
> 
> make check
> (also tested via [zk-mesos](https://github.com/massenz/zk-mesos) that the information
serialized to ZK is readable and as expected).
> 
> Also ran 2x 0.23 master builds against a 0.24 one with this patch applied; getting both
versions in turn to be Leader, and also ran a 0.23 Slave with a 0.24 Leader, and they all
recognized each other.
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>


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