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/07/28 21:44:37 UTC
Review Request 23997: Patch for MESOS-1635
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23997/
-----------------------------------------------------------
Review request for mesos.
Bugs: MESOS-1635
https://issues.apache.org/jira/browse/MESOS-1635
Repository: mesos-git
Description
-------
Fixes MESOS-1635 "zk flag fails when specifying a file and the replicated logs"
replaced the current TODO with code :) The code is consistent with similar types of checks elsewhere in code.
Diffs
-----
src/master/main.cpp 4d39c355eae948ede492d84de2e58a26756b781a
Diff: https://reviews.apache.org/r/23997/diff/
Testing
-------
The code change is simple and no unit test is provided. It compiles, passes style check and is functional via manual check.
Thanks,
Ken Sipe
Re: Review Request 23997: Patch for MESOS-1635
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23997/#review48926
-----------------------------------------------------------
Ship it!
Looks good, just a few minor cleanups and we can get this committed!
src/master/main.cpp
<https://reviews.apache.org/r/23997/#comment85719>
Please use the existing TODO format: add your handle and make it a sentence (end with a period).
src/master/main.cpp
<https://reviews.apache.org/r/23997/#comment85717>
Looks like we should only parse the zk.get() string if it doesn't start with "file://":
Try<URL> url;
if (strings::startsWith(zk.get(), "file://")) {
...
} else {
url = URL::parse(zk.get());
}
src/master/main.cpp
<https://reviews.apache.org/r/23997/#comment85718>
I see this is copied, but this should include the error reason to help the operator:
EXIT(1) << "Failed to read from file at '" + path + "': " << read.error();
- Ben Mahler
On July 28, 2014, 10:36 p.m., Ken Sipe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23997/
> -----------------------------------------------------------
>
> (Updated July 28, 2014, 10:36 p.m.)
>
>
> Review request for mesos, Adam B and Ben Mahler.
>
>
> Bugs: MESOS-1635
> https://issues.apache.org/jira/browse/MESOS-1635
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Fixes MESOS-1635 "zk flag fails when specifying a file and the replicated logs"
>
> replaced the current TODO with code :) The code is consistent with similar types of checks elsewhere in code.
>
>
> Diffs
> -----
>
> src/master/main.cpp 4d39c355eae948ede492d84de2e58a26756b781a
>
> Diff: https://reviews.apache.org/r/23997/diff/
>
>
> Testing
> -------
>
> The code change is simple and no unit test is provided. It compiles, passes style check and is functional via manual check.
>
>
> Thanks,
>
> Ken Sipe
>
>
Re: Review Request 23997: Patch for MESOS-1635
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On July 29, 2014, 3:06 p.m., Jiang Yan Xu wrote:
> > src/master/contender.cpp, line 79
> > <https://reviews.apache.org/r/23997/diff/3/?file=644484#file644484line79>
> >
> > Master detector and contender already support --zk=file://, in the "else if (strings::startsWith(zk, "file://"))" branch below.
> >
> > Looks like the same logic exists in a couple of places it may be reasonable to pull it into "Try<URL> URL::parse(const std::string& url)" directly.
>
> Ben Mahler wrote:
> Good point, that's what this TODO is for but I will clear up the confusion and have a single TODO inside url.hpp.
Oh I see. Cool, thanks :)
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23997/#review49033
-----------------------------------------------------------
On July 29, 2014, 7:33 a.m., Ken Sipe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23997/
> -----------------------------------------------------------
>
> (Updated July 29, 2014, 7:33 a.m.)
>
>
> Review request for mesos, Adam B and Ben Mahler.
>
>
> Bugs: MESOS-1635
> https://issues.apache.org/jira/browse/MESOS-1635
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Fixes MESOS-1635 "zk flag fails when specifying a file and the replicated logs"
>
> replaced the current TODO with code :) The code is consistent with similar types of checks elsewhere in code.
>
>
> Diffs
> -----
>
> src/master/contender.cpp c1bf82b621d6b46afe001acafe9ee53336726406
> src/master/detector.cpp a34cc211941972e2d64802d5c7ed3f5026229744
> src/master/main.cpp 4d39c355eae948ede492d84de2e58a26756b781a
>
> Diff: https://reviews.apache.org/r/23997/diff/
>
>
> Testing
> -------
>
> The code change is simple and no unit test is provided. It compiles, passes style check and is functional via manual check.
>
>
> Thanks,
>
> Ken Sipe
>
>
Re: Review Request 23997: Patch for MESOS-1635
Posted by Ben Mahler <be...@gmail.com>.
> On July 29, 2014, 10:06 p.m., Jiang Yan Xu wrote:
> > src/master/contender.cpp, line 79
> > <https://reviews.apache.org/r/23997/diff/3/?file=644484#file644484line79>
> >
> > Master detector and contender already support --zk=file://, in the "else if (strings::startsWith(zk, "file://"))" branch below.
> >
> > Looks like the same logic exists in a couple of places it may be reasonable to pull it into "Try<URL> URL::parse(const std::string& url)" directly.
Good point, that's what this TODO is for but I will clear up the confusion and have a single TODO inside url.hpp.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23997/#review49033
-----------------------------------------------------------
On July 29, 2014, 2:33 p.m., Ken Sipe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23997/
> -----------------------------------------------------------
>
> (Updated July 29, 2014, 2:33 p.m.)
>
>
> Review request for mesos, Adam B and Ben Mahler.
>
>
> Bugs: MESOS-1635
> https://issues.apache.org/jira/browse/MESOS-1635
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Fixes MESOS-1635 "zk flag fails when specifying a file and the replicated logs"
>
> replaced the current TODO with code :) The code is consistent with similar types of checks elsewhere in code.
>
>
> Diffs
> -----
>
> src/master/contender.cpp c1bf82b621d6b46afe001acafe9ee53336726406
> src/master/detector.cpp a34cc211941972e2d64802d5c7ed3f5026229744
> src/master/main.cpp 4d39c355eae948ede492d84de2e58a26756b781a
>
> Diff: https://reviews.apache.org/r/23997/diff/
>
>
> Testing
> -------
>
> The code change is simple and no unit test is provided. It compiles, passes style check and is functional via manual check.
>
>
> Thanks,
>
> Ken Sipe
>
>
Re: Review Request 23997: Patch for MESOS-1635
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23997/#review49033
-----------------------------------------------------------
src/master/contender.cpp
<https://reviews.apache.org/r/23997/#comment85863>
Master detector and contender already support --zk=file://, in the "else if (strings::startsWith(zk, "file://"))" branch below.
Looks like the same logic exists in a couple of places it may be reasonable to pull it into "Try<URL> URL::parse(const std::string& url)" directly.
src/master/detector.cpp
<https://reviews.apache.org/r/23997/#comment85866>
Ditto
src/master/main.cpp
<https://reviews.apache.org/r/23997/#comment85867>
This is no longer a TODO as it's done now, right?
- Jiang Yan Xu
On July 29, 2014, 7:33 a.m., Ken Sipe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23997/
> -----------------------------------------------------------
>
> (Updated July 29, 2014, 7:33 a.m.)
>
>
> Review request for mesos, Adam B and Ben Mahler.
>
>
> Bugs: MESOS-1635
> https://issues.apache.org/jira/browse/MESOS-1635
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Fixes MESOS-1635 "zk flag fails when specifying a file and the replicated logs"
>
> replaced the current TODO with code :) The code is consistent with similar types of checks elsewhere in code.
>
>
> Diffs
> -----
>
> src/master/contender.cpp c1bf82b621d6b46afe001acafe9ee53336726406
> src/master/detector.cpp a34cc211941972e2d64802d5c7ed3f5026229744
> src/master/main.cpp 4d39c355eae948ede492d84de2e58a26756b781a
>
> Diff: https://reviews.apache.org/r/23997/diff/
>
>
> Testing
> -------
>
> The code change is simple and no unit test is provided. It compiles, passes style check and is functional via manual check.
>
>
> Thanks,
>
> Ken Sipe
>
>
Re: Review Request 23997: Patch for MESOS-1635
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23997/#review49020
-----------------------------------------------------------
Ship it!
Ship It!
- Ben Mahler
On July 29, 2014, 2:33 p.m., Ken Sipe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23997/
> -----------------------------------------------------------
>
> (Updated July 29, 2014, 2:33 p.m.)
>
>
> Review request for mesos, Adam B and Ben Mahler.
>
>
> Bugs: MESOS-1635
> https://issues.apache.org/jira/browse/MESOS-1635
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Fixes MESOS-1635 "zk flag fails when specifying a file and the replicated logs"
>
> replaced the current TODO with code :) The code is consistent with similar types of checks elsewhere in code.
>
>
> Diffs
> -----
>
> src/master/contender.cpp c1bf82b621d6b46afe001acafe9ee53336726406
> src/master/detector.cpp a34cc211941972e2d64802d5c7ed3f5026229744
> src/master/main.cpp 4d39c355eae948ede492d84de2e58a26756b781a
>
> Diff: https://reviews.apache.org/r/23997/diff/
>
>
> Testing
> -------
>
> The code change is simple and no unit test is provided. It compiles, passes style check and is functional via manual check.
>
>
> Thanks,
>
> Ken Sipe
>
>
Re: Review Request 23997: Patch for MESOS-1635
Posted by Ken Sipe <ke...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23997/
-----------------------------------------------------------
(Updated July 29, 2014, 2:33 p.m.)
Review request for mesos, Adam B and Ben Mahler.
Changes
-------
1. todo meets the standard
2. added todo to all places to be refactored
3. added more error information for bad file read
4. added checking for the zk url from the file
5. url parse happens only once
Bugs: MESOS-1635
https://issues.apache.org/jira/browse/MESOS-1635
Repository: mesos-git
Description
-------
Fixes MESOS-1635 "zk flag fails when specifying a file and the replicated logs"
replaced the current TODO with code :) The code is consistent with similar types of checks elsewhere in code.
Diffs (updated)
-----
src/master/contender.cpp c1bf82b621d6b46afe001acafe9ee53336726406
src/master/detector.cpp a34cc211941972e2d64802d5c7ed3f5026229744
src/master/main.cpp 4d39c355eae948ede492d84de2e58a26756b781a
Diff: https://reviews.apache.org/r/23997/diff/
Testing
-------
The code change is simple and no unit test is provided. It compiles, passes style check and is functional via manual check.
Thanks,
Ken Sipe
Re: Review Request 23997: Patch for MESOS-1635
Posted by Ken Sipe <ke...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23997/
-----------------------------------------------------------
(Updated July 28, 2014, 10:36 p.m.)
Review request for mesos, Adam B and Ben Mahler.
Changes
-------
Fixed introduced bug (it is necessary to check for url error after reading it from the file) + add a TODO for some code clean up.
Bugs: MESOS-1635
https://issues.apache.org/jira/browse/MESOS-1635
Repository: mesos-git
Description
-------
Fixes MESOS-1635 "zk flag fails when specifying a file and the replicated logs"
replaced the current TODO with code :) The code is consistent with similar types of checks elsewhere in code.
Diffs (updated)
-----
src/master/main.cpp 4d39c355eae948ede492d84de2e58a26756b781a
Diff: https://reviews.apache.org/r/23997/diff/
Testing
-------
The code change is simple and no unit test is provided. It compiles, passes style check and is functional via manual check.
Thanks,
Ken Sipe
Re: Review Request 23997: Patch for MESOS-1635
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23997/#review48918
-----------------------------------------------------------
Thanks for sending this Ken! There is one small bug that would be nice to fix before getting this committed.
We also might want to leave a TODO here to enhance URL::parse to handle "file://" URLs as well to avoid all the redundant code:
https://github.com/apache/mesos/blob/0.19.1/src/master/detector.cpp#L121
https://github.com/apache/mesos/blob/0.19.1/src/master/contender.cpp#L87
src/master/main.cpp
<https://reviews.apache.org/r/23997/#comment85701>
What if this url is an error?
The code below assumes it is not by calling url.get(), which will crash the program if this url object is an error.
Also please use a 2 space indent as per the rest of this file :)
- Ben Mahler
On July 28, 2014, 7:46 p.m., Ken Sipe wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23997/
> -----------------------------------------------------------
>
> (Updated July 28, 2014, 7:46 p.m.)
>
>
> Review request for mesos, Adam B and Ben Mahler.
>
>
> Bugs: MESOS-1635
> https://issues.apache.org/jira/browse/MESOS-1635
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Fixes MESOS-1635 "zk flag fails when specifying a file and the replicated logs"
>
> replaced the current TODO with code :) The code is consistent with similar types of checks elsewhere in code.
>
>
> Diffs
> -----
>
> src/master/main.cpp 4d39c355eae948ede492d84de2e58a26756b781a
>
> Diff: https://reviews.apache.org/r/23997/diff/
>
>
> Testing
> -------
>
> The code change is simple and no unit test is provided. It compiles, passes style check and is functional via manual check.
>
>
> Thanks,
>
> Ken Sipe
>
>
Re: Review Request 23997: Patch for MESOS-1635
Posted by Ken Sipe <ke...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23997/
-----------------------------------------------------------
(Updated July 28, 2014, 7:46 p.m.)
Review request for mesos, Adam B and Ben Mahler.
Bugs: MESOS-1635
https://issues.apache.org/jira/browse/MESOS-1635
Repository: mesos-git
Description
-------
Fixes MESOS-1635 "zk flag fails when specifying a file and the replicated logs"
replaced the current TODO with code :) The code is consistent with similar types of checks elsewhere in code.
Diffs
-----
src/master/main.cpp 4d39c355eae948ede492d84de2e58a26756b781a
Diff: https://reviews.apache.org/r/23997/diff/
Testing
-------
The code change is simple and no unit test is provided. It compiles, passes style check and is functional via manual check.
Thanks,
Ken Sipe