You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Marco Massenzio <ma...@mesosphere.io> on 2015/07/22 00:50:42 UTC
Review Request 36663: Added ip_address field to MasterInfo
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36663/
-----------------------------------------------------------
Review request for mesos, Anand Mazumdar and Vinod Kone.
Bugs: MESOS-2736
https://issues.apache.org/jira/browse/MESOS-2736
Repository: mesos
Description
-------
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 string format;
this will eventually supersed the int32 existing
`ip`, which also cannot support IPv6 addresses.
Jira: MESOS-2736
Diffs
-----
include/mesos/mesos.proto bcb38d934c7f223b9a23746b273e581e0e8da886
src/common/protobuf_utils.cpp 9ba57a73e44ddbebfc44d0de61ebefd1ab620209
src/master/master.cpp 2f00f240ed2cd59ec0c2eae7fd2567f0edb8d9e0
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).
Thanks,
Marco Massenzio
Re: Review Request 36663: Added ip_address field to MasterInfo
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36663/#review92630
-----------------------------------------------------------
Can you write a test for this?
include/mesos/mesos.proto (line 374)
<https://reviews.apache.org/r/36663/#comment146852>
s/ip_address/address/
see below.
include/mesos/mesos.proto (line 378)
<https://reviews.apache.org/r/36663/#comment146845>
s/; can be configured using the --port flag//
Mention that this is deprecated in favor of 'address'.
include/mesos/mesos.proto (line 389)
<https://reviews.apache.org/r/36663/#comment146846>
s/eg/e.g./
include/mesos/mesos.proto (line 390)
<https://reviews.apache.org/r/36663/#comment146853>
Mention that this is deprecated in favor of 'address'.
include/mesos/mesos.proto (line 398)
<https://reviews.apache.org/r/36663/#comment146847>
Lets use the newly created Address protobuf for this.
optional Address address;
The goal is to use this protobuf for conveying address of a component going forward. We are already using this in Offer.
src/common/protobuf_utils.cpp (lines 185 - 191)
<https://reviews.apache.org/r/36663/#comment146855>
See how we set 'address' protobuf in Master::offer() and do thte same.
src/master/master.cpp (lines 326 - 328)
<https://reviews.apache.org/r/36663/#comment146856>
ditto.
- Vinod Kone
On July 21, 2015, 10:50 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36663/
> -----------------------------------------------------------
>
> (Updated July 21, 2015, 10:50 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Vinod Kone.
>
>
> Bugs: MESOS-2736
> https://issues.apache.org/jira/browse/MESOS-2736
>
>
> Repository: mesos
>
>
> Description
> -------
>
> 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 string format;
> this will eventually supersed the int32 existing
> `ip`, which also cannot support IPv6 addresses.
>
> Jira: MESOS-2736
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto bcb38d934c7f223b9a23746b273e581e0e8da886
> src/common/protobuf_utils.cpp 9ba57a73e44ddbebfc44d0de61ebefd1ab620209
> src/master/master.cpp 2f00f240ed2cd59ec0c2eae7fd2567f0edb8d9e0
>
> 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).
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 36663: Added ip_address field to MasterInfo
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On July 23, 2015, 11:51 p.m., Vinod Kone wrote:
> > src/tests/master_tests.cpp, line 1022
> > <https://reviews.apache.org/r/36663/diff/2/?file=1020111#file1020111line1022>
> >
> > This test is testing the createMasterInfo() method which is not used in production. Can you also write a test with ZK in play? That is going to test the changes you made in master.cpp.
Yep - I had looked in the zookeeper_* tests, but found nothing related to MasterInfo; turns out the tests are in the master_contender.cpp tests.
Added one there.
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36663/#review92834
-----------------------------------------------------------
On July 23, 2015, 11:54 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36663/
> -----------------------------------------------------------
>
> (Updated July 23, 2015, 11:54 p.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
> -----
>
> include/mesos/mesos.proto e015c81d5052214ef8207642e23b3892a6123c9a
> include/mesos/scheduler/scheduler.proto 89daf8a6b74057ee156b3ad691397e76fcb835b8
> src/common/protobuf_utils.cpp e0f82b53f5e106bbf4e21d6ac946df0fae821882
> src/master/master.hpp bf61bb2deb61a73303f4122383dcb7e8f5d726de
> src/master/master.cpp bab04feaf238e51c4d50feb1d3a9c0735ec68fec
> src/scheduler/scheduler.cpp badc107dbf4e2bd9146f9244724e0db5c2ae05d3
> src/tests/master_tests.cpp 826f276317da2507cdf99b2f2e21a147444469d8
> src/tests/scheduler_tests.cpp 98fc70bf43ba99b54064a236795c7e1269004b71
>
> 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
>
>
Re: Review Request 36663: Added ip_address field to MasterInfo
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On July 23, 2015, 11:51 p.m., Vinod Kone wrote:
> > src/tests/master_tests.cpp, line 1022
> > <https://reviews.apache.org/r/36663/diff/2/?file=1020111#file1020111line1022>
> >
> > This test is testing the createMasterInfo() method which is not used in production. Can you also write a test with ZK in play? That is going to test the changes you made in master.cpp.
>
> Marco Massenzio wrote:
> Yep - I had looked in the zookeeper_* tests, but found nothing related to MasterInfo; turns out the tests are in the master_contender.cpp tests.
> Added one there.
Actually, the test is against the 'basic contender' (which, I understand, is not against ZK) - I am assuming this is good enough? we are not really testing the ZK functionality here, only that MasterInfo gets created with the correct data.
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36663/#review92834
-----------------------------------------------------------
On July 24, 2015, 12:17 a.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36663/
> -----------------------------------------------------------
>
> (Updated July 24, 2015, 12:17 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
> -----
>
> include/mesos/mesos.proto e015c81d5052214ef8207642e23b3892a6123c9a
> src/common/protobuf_utils.cpp e0f82b53f5e106bbf4e21d6ac946df0fae821882
> src/master/master.cpp bab04feaf238e51c4d50feb1d3a9c0735ec68fec
> src/tests/master_contender_detector_tests.cpp d7a3b46b2e437818631064ae34317e49c9aa3748
> src/tests/master_tests.cpp 826f276317da2507cdf99b2f2e21a147444469d8
>
> 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
>
>
Re: Review Request 36663: Added ip_address field to MasterInfo
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36663/#review92834
-----------------------------------------------------------
src/master/master.cpp (lines 344 - 346)
<https://reviews.apache.org/r/36663/#comment147078>
Move this up to #319.
src/tests/master_tests.cpp (line 1019)
<https://reviews.apache.org/r/36663/#comment147080>
why not compare ip with master.get().address.ip() ?
mesos doesn't necessarily bind to 127.0.0.1 as you might've seen in reviewbot failure.
src/tests/master_tests.cpp (line 1021)
<https://reviews.apache.org/r/36663/#comment147082>
ditto. doesn't necessarily map to "localhost".
src/tests/master_tests.cpp (line 1022)
<https://reviews.apache.org/r/36663/#comment147083>
This test is testing the createMasterInfo() method which is not used in production. Can you also write a test with ZK in play? That is going to test the changes you made in master.cpp.
- Vinod Kone
On July 23, 2015, 9:46 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36663/
> -----------------------------------------------------------
>
> (Updated July 23, 2015, 9:46 p.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
> -----
>
> include/mesos/mesos.proto bcb38d934c7f223b9a23746b273e581e0e8da886
> src/common/protobuf_utils.cpp e0f82b53f5e106bbf4e21d6ac946df0fae821882
> src/master/master.cpp 2f00f240ed2cd59ec0c2eae7fd2567f0edb8d9e0
> src/tests/master_tests.cpp 826f276317da2507cdf99b2f2e21a147444469d8
>
> 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
>
>
Re: Review Request 36663: Added ip_address field to MasterInfo
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36663/#review92958
-----------------------------------------------------------
include/mesos/mesos.proto (line 397)
<https://reviews.apache.org/r/36663/#comment147227>
Can you please add these deprecation warnings to 0.24.0 CHANGELOG?
```
Release Notes - Mesos - Version 0.24.0 (WIP)
* Deprecations
<blah>
```
src/common/protobuf_utils.cpp (line 194)
<https://reviews.apache.org/r/36663/#comment147229>
s/remove/Remove/
src/master/master.cpp (line 320)
<https://reviews.apache.org/r/36663/#comment147230>
s/the/The/
src/tests/master_tests.cpp (line 995)
<https://reviews.apache.org/r/36663/#comment147246>
Make this a MasterZooKeeperTest so that we can use zookeeper detector here and test Master::Master() changes!
TEST_F(MasterZooKeeperTest, MasterInfoAddress)
you'll want to move this to #2163
src/tests/master_tests.cpp (line 1023)
<https://reviews.apache.org/r/36663/#comment147233>
new line.
src/tests/master_tests.cpp (line 1024)
<https://reviews.apache.org/r/36663/#comment147234>
period at the end.
- Vinod Kone
On July 24, 2015, 12:17 a.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36663/
> -----------------------------------------------------------
>
> (Updated July 24, 2015, 12:17 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
> -----
>
> include/mesos/mesos.proto e015c81d5052214ef8207642e23b3892a6123c9a
> src/common/protobuf_utils.cpp e0f82b53f5e106bbf4e21d6ac946df0fae821882
> src/master/master.cpp bab04feaf238e51c4d50feb1d3a9c0735ec68fec
> src/tests/master_contender_detector_tests.cpp d7a3b46b2e437818631064ae34317e49c9aa3748
> src/tests/master_tests.cpp 826f276317da2507cdf99b2f2e21a147444469d8
>
> 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
>
>
Re: Review Request 36663: Added ip_address field to MasterInfo
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36663/#review92853
-----------------------------------------------------------
Patch looks great!
Reviews applied: [36663]
All tests passed.
- Mesos ReviewBot
On July 24, 2015, 12:17 a.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36663/
> -----------------------------------------------------------
>
> (Updated July 24, 2015, 12:17 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
> -----
>
> include/mesos/mesos.proto e015c81d5052214ef8207642e23b3892a6123c9a
> src/common/protobuf_utils.cpp e0f82b53f5e106bbf4e21d6ac946df0fae821882
> src/master/master.cpp bab04feaf238e51c4d50feb1d3a9c0735ec68fec
> src/tests/master_contender_detector_tests.cpp d7a3b46b2e437818631064ae34317e49c9aa3748
> src/tests/master_tests.cpp 826f276317da2507cdf99b2f2e21a147444469d8
>
> 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
>
>
Re: Review Request 36663: Added ip_address field to MasterInfo
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36663/#review93017
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
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
>
>
Re: Review Request 36663: Added ip_address field to MasterInfo
Posted by Alexander Rukletsov <ru...@gmail.com>.
> 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
>
>
Re: Review Request 36663: Added ip_address field to MasterInfo
Posted by Marco Massenzio <ma...@mesosphere.io>.
> 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?
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.
- Marco
-----------------------------------------------------------
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
>
>
Re: Review Request 36663: Added ip_address field to MasterInfo
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36663/#review93441
-----------------------------------------------------------
include/mesos/mesos.proto (line 399)
<https://reviews.apache.org/r/36663/#comment147803>
`ip` and `port` are required, while `address` is optional. Is it intentional / doesn't it introduce a pitfall?
- Alexander Rukletsov
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
>
>
Re: Review Request 36663: Added ip_address field to MasterInfo
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
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.
Changes
-------
removed the test - will be part of another review (it causes tests to hang in ZK)
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 (updated)
-----
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
Re: Review Request 36663: Added ip_address field to MasterInfo
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36663/
-----------------------------------------------------------
(Updated July 24, 2015, 12:17 a.m.)
Review request for mesos, Anand Mazumdar and Vinod Kone.
Changes
-------
Added test against ZK.
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 (updated)
-----
include/mesos/mesos.proto e015c81d5052214ef8207642e23b3892a6123c9a
src/common/protobuf_utils.cpp e0f82b53f5e106bbf4e21d6ac946df0fae821882
src/master/master.cpp bab04feaf238e51c4d50feb1d3a9c0735ec68fec
src/tests/master_contender_detector_tests.cpp d7a3b46b2e437818631064ae34317e49c9aa3748
src/tests/master_tests.cpp 826f276317da2507cdf99b2f2e21a147444469d8
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
Re: Review Request 36663: Added ip_address field to MasterInfo
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36663/
-----------------------------------------------------------
(Updated July 23, 2015, 11:54 p.m.)
Review request for mesos, Anand Mazumdar and Vinod Kone.
Changes
-------
Fixed test failure
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 (updated)
-----
include/mesos/mesos.proto e015c81d5052214ef8207642e23b3892a6123c9a
include/mesos/scheduler/scheduler.proto 89daf8a6b74057ee156b3ad691397e76fcb835b8
src/common/protobuf_utils.cpp e0f82b53f5e106bbf4e21d6ac946df0fae821882
src/master/master.hpp bf61bb2deb61a73303f4122383dcb7e8f5d726de
src/master/master.cpp bab04feaf238e51c4d50feb1d3a9c0735ec68fec
src/scheduler/scheduler.cpp badc107dbf4e2bd9146f9244724e0db5c2ae05d3
src/tests/master_tests.cpp 826f276317da2507cdf99b2f2e21a147444469d8
src/tests/scheduler_tests.cpp 98fc70bf43ba99b54064a236795c7e1269004b71
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
Re: Review Request 36663: Added ip_address field to MasterInfo
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36663/
-----------------------------------------------------------
(Updated July 23, 2015, 9:46 p.m.)
Review request for mesos, Anand Mazumdar and Vinod Kone.
Changes
-------
Update description to reflect commit log
Bugs: MESOS-2736
https://issues.apache.org/jira/browse/MESOS-2736
Repository: mesos
Description (updated)
-------
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
-----
include/mesos/mesos.proto bcb38d934c7f223b9a23746b273e581e0e8da886
src/common/protobuf_utils.cpp e0f82b53f5e106bbf4e21d6ac946df0fae821882
src/master/master.cpp 2f00f240ed2cd59ec0c2eae7fd2567f0edb8d9e0
src/tests/master_tests.cpp 826f276317da2507cdf99b2f2e21a147444469d8
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
Re: Review Request 36663: Added ip_address field to MasterInfo
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36663/
-----------------------------------------------------------
(Updated July 23, 2015, 9:33 p.m.)
Review request for mesos, Anand Mazumdar and Vinod Kone.
Changes
-------
Ran some spot check across versions
Bugs: MESOS-2736
https://issues.apache.org/jira/browse/MESOS-2736
Repository: mesos
Description
-------
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 string format;
this will eventually supersed the int32 existing
`ip`, which also cannot support IPv6 addresses.
Jira: MESOS-2736
Diffs
-----
include/mesos/mesos.proto bcb38d934c7f223b9a23746b273e581e0e8da886
src/common/protobuf_utils.cpp e0f82b53f5e106bbf4e21d6ac946df0fae821882
src/master/master.cpp 2f00f240ed2cd59ec0c2eae7fd2567f0edb8d9e0
src/tests/master_tests.cpp 826f276317da2507cdf99b2f2e21a147444469d8
Diff: https://reviews.apache.org/r/36663/diff/
Testing (updated)
-------
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
Re: Review Request 36663: Added ip_address field to MasterInfo
Posted by Marco Massenzio <ma...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36663/
-----------------------------------------------------------
(Updated July 23, 2015, 8:29 p.m.)
Review request for mesos, Anand Mazumdar and Vinod Kone.
Changes
-------
Refactored to use Address; added a unit test.
Bugs: MESOS-2736
https://issues.apache.org/jira/browse/MESOS-2736
Repository: mesos
Description
-------
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 string format;
this will eventually supersed the int32 existing
`ip`, which also cannot support IPv6 addresses.
Jira: MESOS-2736
Diffs (updated)
-----
include/mesos/mesos.proto bcb38d934c7f223b9a23746b273e581e0e8da886
src/common/protobuf_utils.cpp e0f82b53f5e106bbf4e21d6ac946df0fae821882
src/master/master.cpp 2f00f240ed2cd59ec0c2eae7fd2567f0edb8d9e0
src/tests/master_tests.cpp 826f276317da2507cdf99b2f2e21a147444469d8
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).
Thanks,
Marco Massenzio
Re: Review Request 36663: Added ip_address field to MasterInfo
Posted by Marco Massenzio <ma...@mesosphere.io>.
> On July 21, 2015, 11:59 p.m., Anand Mazumdar wrote:
> > LGTM, thanks for adding much needed documentation too for the fields !
> >
> > Upon reading the corresponding JIRA, I am assuming that we had an agreement to not use the Address field that BenM introduced in r36450 for readability of the field.
Well, I summarized the alternatives and put forward my recommendation - that was on 6/15: there was no further comment since, so I acted on the principle of "lazy consensus".
Also, please note that this will be serialized in JSON and the users of this are largely assumed NOT to use libmesos, so the benefits of the `Address` protobuf are very limited (in fact, it makes the generated JSON more difficult to parse and the entire structure more confusing; as I articulated in MESOS-2736) as we cannot remove existing fields without breaking backward compatibility, and the information would be repeated in difference places making the whole thing mightily confusing.
> On July 21, 2015, 11:59 p.m., Anand Mazumdar wrote:
> > include/mesos/mesos.proto, line 382
> > <https://reviews.apache.org/r/36663/diff/1/?file=1018235#file1018235line382>
> >
> > Minor Nitpick : Did you refer to a "name;" here instead of "name:"
Thanks, you're right `;` works better.
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36663/#review92514
-----------------------------------------------------------
On July 21, 2015, 10:50 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36663/
> -----------------------------------------------------------
>
> (Updated July 21, 2015, 10:50 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Vinod Kone.
>
>
> Bugs: MESOS-2736
> https://issues.apache.org/jira/browse/MESOS-2736
>
>
> Repository: mesos
>
>
> Description
> -------
>
> 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 string format;
> this will eventually supersed the int32 existing
> `ip`, which also cannot support IPv6 addresses.
>
> Jira: MESOS-2736
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto bcb38d934c7f223b9a23746b273e581e0e8da886
> src/common/protobuf_utils.cpp 9ba57a73e44ddbebfc44d0de61ebefd1ab620209
> src/master/master.cpp 2f00f240ed2cd59ec0c2eae7fd2567f0edb8d9e0
>
> 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).
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 36663: Added ip_address field to MasterInfo
Posted by Anand Mazumdar <ma...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36663/#review92514
-----------------------------------------------------------
Ship it!
LGTM, thanks for adding much needed documentation too for the fields !
Upon reading the corresponding JIRA, I am assuming that we had an agreement to not use the Address field that BenM introduced in r36450 for readability of the field.
include/mesos/mesos.proto (line 382)
<https://reviews.apache.org/r/36663/#comment146724>
Minor Nitpick : Did you refer to a "name;" here instead of "name:"
- Anand Mazumdar
On July 21, 2015, 10:50 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36663/
> -----------------------------------------------------------
>
> (Updated July 21, 2015, 10:50 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Vinod Kone.
>
>
> Bugs: MESOS-2736
> https://issues.apache.org/jira/browse/MESOS-2736
>
>
> Repository: mesos
>
>
> Description
> -------
>
> 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 string format;
> this will eventually supersed the int32 existing
> `ip`, which also cannot support IPv6 addresses.
>
> Jira: MESOS-2736
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto bcb38d934c7f223b9a23746b273e581e0e8da886
> src/common/protobuf_utils.cpp 9ba57a73e44ddbebfc44d0de61ebefd1ab620209
> src/master/master.cpp 2f00f240ed2cd59ec0c2eae7fd2567f0edb8d9e0
>
> 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).
>
>
> Thanks,
>
> Marco Massenzio
>
>
Re: Review Request 36663: Added ip_address field to MasterInfo
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36663/#review92510
-----------------------------------------------------------
Patch looks great!
Reviews applied: [36663]
All tests passed.
- Mesos ReviewBot
On July 21, 2015, 10:50 p.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36663/
> -----------------------------------------------------------
>
> (Updated July 21, 2015, 10:50 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Vinod Kone.
>
>
> Bugs: MESOS-2736
> https://issues.apache.org/jira/browse/MESOS-2736
>
>
> Repository: mesos
>
>
> Description
> -------
>
> 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 string format;
> this will eventually supersed the int32 existing
> `ip`, which also cannot support IPv6 addresses.
>
> Jira: MESOS-2736
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto bcb38d934c7f223b9a23746b273e581e0e8da886
> src/common/protobuf_utils.cpp 9ba57a73e44ddbebfc44d0de61ebefd1ab620209
> src/master/master.cpp 2f00f240ed2cd59ec0c2eae7fd2567f0edb8d9e0
>
> 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).
>
>
> Thanks,
>
> Marco Massenzio
>
>