You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ken Sipe <ke...@gmail.com> on 2014/08/11 18:44:52 UTC
Review Request 24562: ZK URL Parsing Consolidation
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24562/
-----------------------------------------------------------
Review request for mesos, Adam B and Ben Mahler.
Repository: mesos-git
Description
-------
Consolidated redundant code for zk url parsing. Through that, identified a test that used zk file for something other than zk. Fixed test, and added test for file parsing. It is tough to know if something other than zk can be in the file (assuming it is a zk file). If other things can go into the file in question, then I can create a file parser. The current URL::Parser lives in the zookeeper namespace.
This patch also completes several documented todo is the code.
Diffs
-----
src/master/contender.cpp c1bf82b621d6b46afe001acafe9ee53336726406
src/master/detector.cpp 6436b8ee7e1ab6451a6b999a1cfbb2f79190e6ca
src/tests/master_contender_detector_tests.cpp fdddfa1a9c793bb406d85d214a18ca4ced53b09f
src/zookeeper/url.hpp 16e711c5c0bc29b1967a20f0827238f8a7b0deaf
Diff: https://reviews.apache.org/r/24562/diff/
Testing
-------
1 test changed and 1 test added to: master_contender_detector_tests.cpp
MasterContenderDetectorTest
BasicMasterContenderDetectorTest
all tests pass including styling.
Thanks,
Ken Sipe
Re: Review Request 24562: ZK URL Parsing Consolidation
Posted by Ken Sipe <ke...@gmail.com>.
> On Aug. 11, 2014, 8:18 p.m., Benjamin Hindman wrote:
> > src/master/detector.cpp, line 201
> > <https://reviews.apache.org/r/24562/diff/2/?file=657967#file657967line201>
> >
> > The original intent _was_ to enable putting something like 'ip:port' in the file, which this change no longer supports, but I don't actually know if that is being used anywhere in production. :-(
could you (or someone) provide all the options that are valid in the file. IF this is the case, this isn't a zk file (as I assumed) and whatever is in the the file will need to go through URL processing after it is pulled out of the file. The options I know are (zk:// and now master@). If you want to support ip:port, then I assume that this is a master@. Are there more?
master@10.0.0.2:54068
zk://10.0.0.2:2181/mesos
10.0.0.2:54068 -> which is a master@ short-hand?
there are 3 conflated ideas in this area of code which I would like to tease apart. the 3 things are, 1) in file or not, 2) url connections, and 3) ZookeeperDetectors vs. StandaloneMasterDetector. (all 3 concepts are not present in all usages of URL::parse)
- Ken
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24562/#review50222
-----------------------------------------------------------
On Aug. 11, 2014, 7:54 p.m., Ken Sipe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24562/
> -----------------------------------------------------------
>
> (Updated Aug. 11, 2014, 7:54 p.m.)
>
>
> Review request for mesos, Adam B and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Consolidated redundant code for zk url parsing. Through that, identified a test that used zk file for something other than zk. Fixed test, and added test for file parsing. It is tough to know if something other than zk can be in the file (assuming it is a zk file). If other things can go into the file in question, then I can create a file parser. The current URL::Parser lives in the zookeeper namespace.
>
> This patch also completes several documented todo is the code.
>
>
> Diffs
> -----
>
> src/master/contender.cpp c1bf82b621d6b46afe001acafe9ee53336726406
> src/master/detector.cpp 6436b8ee7e1ab6451a6b999a1cfbb2f79190e6ca
> src/tests/master_contender_detector_tests.cpp fdddfa1a9c793bb406d85d214a18ca4ced53b09f
> src/zookeeper/url.hpp 16e711c5c0bc29b1967a20f0827238f8a7b0deaf
>
> Diff: https://reviews.apache.org/r/24562/diff/
>
>
> Testing
> -------
>
> 1 test changed and 1 test added to: master_contender_detector_tests.cpp
> MasterContenderDetectorTest
> BasicMasterContenderDetectorTest
>
> all tests pass including styling.
>
>
> Thanks,
>
> Ken Sipe
>
>
Re: Review Request 24562: ZK URL Parsing Consolidation
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24562/#review50222
-----------------------------------------------------------
Just a quick drive by review here, nothing complete.
src/master/detector.cpp
<https://reviews.apache.org/r/24562/#comment87871>
The original intent _was_ to enable putting something like 'ip:port' in the file, which this change no longer supports, but I don't actually know if that is being used anywhere in production. :-(
- Benjamin Hindman
On Aug. 11, 2014, 7:54 p.m., Ken Sipe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24562/
> -----------------------------------------------------------
>
> (Updated Aug. 11, 2014, 7:54 p.m.)
>
>
> Review request for mesos, Adam B and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Consolidated redundant code for zk url parsing. Through that, identified a test that used zk file for something other than zk. Fixed test, and added test for file parsing. It is tough to know if something other than zk can be in the file (assuming it is a zk file). If other things can go into the file in question, then I can create a file parser. The current URL::Parser lives in the zookeeper namespace.
>
> This patch also completes several documented todo is the code.
>
>
> Diffs
> -----
>
> src/master/contender.cpp c1bf82b621d6b46afe001acafe9ee53336726406
> src/master/detector.cpp 6436b8ee7e1ab6451a6b999a1cfbb2f79190e6ca
> src/tests/master_contender_detector_tests.cpp fdddfa1a9c793bb406d85d214a18ca4ced53b09f
> src/zookeeper/url.hpp 16e711c5c0bc29b1967a20f0827238f8a7b0deaf
>
> Diff: https://reviews.apache.org/r/24562/diff/
>
>
> Testing
> -------
>
> 1 test changed and 1 test added to: master_contender_detector_tests.cpp
> MasterContenderDetectorTest
> BasicMasterContenderDetectorTest
>
> all tests pass including styling.
>
>
> Thanks,
>
> Ken Sipe
>
>
Re: Review Request 24562: ZK URL Parsing Consolidation
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24562/#review50390
-----------------------------------------------------------
Patch looks great!
Reviews applied: [24562]
All tests passed.
- Mesos ReviewBot
On Aug. 11, 2014, 7:54 p.m., Ken Sipe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24562/
> -----------------------------------------------------------
>
> (Updated Aug. 11, 2014, 7:54 p.m.)
>
>
> Review request for mesos, Adam B and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Consolidated redundant code for zk url parsing. Through that, identified a test that used zk file for something other than zk. Fixed test, and added test for file parsing. It is tough to know if something other than zk can be in the file (assuming it is a zk file). If other things can go into the file in question, then I can create a file parser. The current URL::Parser lives in the zookeeper namespace.
>
> This patch also completes several documented todo is the code.
>
>
> Diffs
> -----
>
> src/master/contender.cpp c1bf82b621d6b46afe001acafe9ee53336726406
> src/master/detector.cpp 6436b8ee7e1ab6451a6b999a1cfbb2f79190e6ca
> src/tests/master_contender_detector_tests.cpp fdddfa1a9c793bb406d85d214a18ca4ced53b09f
> src/zookeeper/url.hpp 16e711c5c0bc29b1967a20f0827238f8a7b0deaf
>
> Diff: https://reviews.apache.org/r/24562/diff/
>
>
> Testing
> -------
>
> 1 test changed and 1 test added to: master_contender_detector_tests.cpp
> MasterContenderDetectorTest
> BasicMasterContenderDetectorTest
>
> all tests pass including styling.
>
>
> Thanks,
>
> Ken Sipe
>
>
Re: Review Request 24562: ZK URL Parsing Consolidation
Posted by Ken Sipe <ke...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24562/
-----------------------------------------------------------
(Updated Aug. 11, 2014, 7:54 p.m.)
Review request for mesos, Adam B and Ben Mahler.
Changes
-------
good catch. I'm not sure how that was introduced and style check didn't catch.
Repository: mesos-git
Description
-------
Consolidated redundant code for zk url parsing. Through that, identified a test that used zk file for something other than zk. Fixed test, and added test for file parsing. It is tough to know if something other than zk can be in the file (assuming it is a zk file). If other things can go into the file in question, then I can create a file parser. The current URL::Parser lives in the zookeeper namespace.
This patch also completes several documented todo is the code.
Diffs (updated)
-----
src/master/contender.cpp c1bf82b621d6b46afe001acafe9ee53336726406
src/master/detector.cpp 6436b8ee7e1ab6451a6b999a1cfbb2f79190e6ca
src/tests/master_contender_detector_tests.cpp fdddfa1a9c793bb406d85d214a18ca4ced53b09f
src/zookeeper/url.hpp 16e711c5c0bc29b1967a20f0827238f8a7b0deaf
Diff: https://reviews.apache.org/r/24562/diff/
Testing
-------
1 test changed and 1 test added to: master_contender_detector_tests.cpp
MasterContenderDetectorTest
BasicMasterContenderDetectorTest
all tests pass including styling.
Thanks,
Ken Sipe
Re: Review Request 24562: ZK URL Parsing Consolidation
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24562/#review50187
-----------------------------------------------------------
src/tests/master_contender_detector_tests.cpp
<https://reviews.apache.org/r/24562/#comment87793>
style only: some extra whitespace crept in here - please remove it.
- Dominic Hamon
On Aug. 11, 2014, 9:44 a.m., Ken Sipe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24562/
> -----------------------------------------------------------
>
> (Updated Aug. 11, 2014, 9:44 a.m.)
>
>
> Review request for mesos, Adam B and Ben Mahler.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Consolidated redundant code for zk url parsing. Through that, identified a test that used zk file for something other than zk. Fixed test, and added test for file parsing. It is tough to know if something other than zk can be in the file (assuming it is a zk file). If other things can go into the file in question, then I can create a file parser. The current URL::Parser lives in the zookeeper namespace.
>
> This patch also completes several documented todo is the code.
>
>
> Diffs
> -----
>
> src/master/contender.cpp c1bf82b621d6b46afe001acafe9ee53336726406
> src/master/detector.cpp 6436b8ee7e1ab6451a6b999a1cfbb2f79190e6ca
> src/tests/master_contender_detector_tests.cpp fdddfa1a9c793bb406d85d214a18ca4ced53b09f
> src/zookeeper/url.hpp 16e711c5c0bc29b1967a20f0827238f8a7b0deaf
>
> Diff: https://reviews.apache.org/r/24562/diff/
>
>
> Testing
> -------
>
> 1 test changed and 1 test added to: master_contender_detector_tests.cpp
> MasterContenderDetectorTest
> BasicMasterContenderDetectorTest
>
> all tests pass including styling.
>
>
> Thanks,
>
> Ken Sipe
>
>