Re: Review Request 36663: Added ip_address field to MasterInfo
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
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
--- 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
--- 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 826f276317da2507cdf99b2f2e21a14769d8 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
--- 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
--- 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
--- 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 826f276317da2507cdf99b2f2e21a14769d8 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
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 826f276317da2507cdf99b2f2e21a14769d8 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
--- 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 826f276317da2507cdf99b2f2e21a14769d8 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
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 826f276317da2507cdf99b2f2e21a14769d8 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
--- 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 826f276317da2507cdf99b2f2e21a14769d8 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
--- 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 826f276317da2507cdf99b2f2e21a14769d8 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
--- 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 826f276317da2507cdf99b2f2e21a14769d8 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
--- 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 826f276317da2507cdf99b2f2e21a14769d8 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
--- 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
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
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
--- 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
Re: 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/#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
