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