You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Brenden Matthews <br...@diddyinc.com> on 2013/07/24 01:42:50 UTC
Review Request 12885: Slave ping timeout may be increased by flags.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12885/
-----------------------------------------------------------
Review request for mesos.
Repository: mesos-git
Description
-------
Slave ping timeout may be increased by flags.
Review: https://reviews.apache.org/r/12885
Diffs
-----
src/master/constants.hpp f3a95f1006472ff2b90cabb8cf74182830740954
src/master/constants.cpp 7bc32eb3bdae717ded63436e47031934384eec23
src/master/flags.hpp 6dbbb79d447991a4929fe27628961c0874adadf8
src/master/master.cpp e4507acd97ebaf2ce693ec7343e9a4a563f6ff80
src/tests/fault_tolerance_tests.cpp c8d88d5f60cf49ef5c1ffa429d308c30c2e5005c
Diff: https://reviews.apache.org/r/12885/diff/
Testing
-------
make check
Thanks,
Brenden Matthews
Re: Review Request 12885: Slave ping timeout may be increased by
flags.
Posted by Brenden Matthews <br...@diddyinc.com>.
> On May 22, 2015, 3:56 p.m., Niklas Nielsen wrote:
> > Is this obsoleted by https://reviews.apache.org/r/29507/?
Yes, I believe so.
- Brenden
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12885/#review84921
-----------------------------------------------------------
On July 30, 2013, 11:45 p.m., Brenden Matthews wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12885/
> -----------------------------------------------------------
>
> (Updated July 30, 2013, 11:45 p.m.)
>
>
> Review request for mesos.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Slave ping timeout may be increased by flags.
>
> Review: https://reviews.apache.org/r/12885
>
>
> Diffs
> -----
>
> src/master/constants.hpp f3a95f1006472ff2b90cabb8cf74182830740954
> src/master/constants.cpp 7bc32eb3bdae717ded63436e47031934384eec23
> src/master/flags.hpp 6dbbb79d447991a4929fe27628961c0874adadf8
> src/master/master.cpp e4507acd97ebaf2ce693ec7343e9a4a563f6ff80
> src/tests/fault_tolerance_tests.cpp c8d88d5f60cf49ef5c1ffa429d308c30c2e5005c
>
> Diff: https://reviews.apache.org/r/12885/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Brenden Matthews
>
>
Re: Review Request 12885: Slave ping timeout may be increased by
flags.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12885/#review84921
-----------------------------------------------------------
Is this obsoleted by https://reviews.apache.org/r/29507/?
- Niklas Nielsen
On July 30, 2013, 4:45 p.m., Brenden Matthews wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12885/
> -----------------------------------------------------------
>
> (Updated July 30, 2013, 4:45 p.m.)
>
>
> Review request for mesos.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Slave ping timeout may be increased by flags.
>
> Review: https://reviews.apache.org/r/12885
>
>
> Diffs
> -----
>
> src/master/constants.hpp f3a95f1006472ff2b90cabb8cf74182830740954
> src/master/constants.cpp 7bc32eb3bdae717ded63436e47031934384eec23
> src/master/flags.hpp 6dbbb79d447991a4929fe27628961c0874adadf8
> src/master/master.cpp e4507acd97ebaf2ce693ec7343e9a4a563f6ff80
> src/tests/fault_tolerance_tests.cpp c8d88d5f60cf49ef5c1ffa429d308c30c2e5005c
>
> Diff: https://reviews.apache.org/r/12885/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Brenden Matthews
>
>
Re: Review Request 12885: Slave ping timeout may be increased by
flags.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12885/#review84939
-----------------------------------------------------------
Bad patch!
Reviews applied: [12885]
Failed command: ./support/apply-review.sh -n -r 12885
Error:
2015-05-22 17:24:55 URL:https://reviews.apache.org/r/12885/diff/raw/ [6806/6806] -> "12885.patch" [1]
error: patch failed: src/master/constants.hpp:52
error: src/master/constants.hpp: patch does not apply
error: patch failed: src/master/constants.cpp:29
error: src/master/constants.cpp: patch does not apply
error: patch failed: src/master/flags.hpp:25
error: src/master/flags.hpp: patch does not apply
error: patch failed: src/master/master.cpp:131
error: src/master/master.cpp: patch does not apply
error: patch failed: src/tests/fault_tolerance_tests.cpp:177
error: src/tests/fault_tolerance_tests.cpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On July 30, 2013, 11:45 p.m., Brenden Matthews wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12885/
> -----------------------------------------------------------
>
> (Updated July 30, 2013, 11:45 p.m.)
>
>
> Review request for mesos.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Slave ping timeout may be increased by flags.
>
> Review: https://reviews.apache.org/r/12885
>
>
> Diffs
> -----
>
> src/master/constants.hpp f3a95f1006472ff2b90cabb8cf74182830740954
> src/master/constants.cpp 7bc32eb3bdae717ded63436e47031934384eec23
> src/master/flags.hpp 6dbbb79d447991a4929fe27628961c0874adadf8
> src/master/master.cpp e4507acd97ebaf2ce693ec7343e9a4a563f6ff80
> src/tests/fault_tolerance_tests.cpp c8d88d5f60cf49ef5c1ffa429d308c30c2e5005c
>
> Diff: https://reviews.apache.org/r/12885/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Brenden Matthews
>
>
Re: Review Request 12885: Slave ping timeout may be increased by flags.
Posted by Brenden Matthews <br...@diddyinc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12885/
-----------------------------------------------------------
(Updated July 30, 2013, 11:45 p.m.)
Review request for mesos.
Changes
-------
Updated as per Ben's comments.
Repository: mesos-git
Description
-------
Slave ping timeout may be increased by flags.
Review: https://reviews.apache.org/r/12885
Diffs (updated)
-----
src/master/constants.hpp f3a95f1006472ff2b90cabb8cf74182830740954
src/master/constants.cpp 7bc32eb3bdae717ded63436e47031934384eec23
src/master/flags.hpp 6dbbb79d447991a4929fe27628961c0874adadf8
src/master/master.cpp e4507acd97ebaf2ce693ec7343e9a4a563f6ff80
src/tests/fault_tolerance_tests.cpp c8d88d5f60cf49ef5c1ffa429d308c30c2e5005c
Diff: https://reviews.apache.org/r/12885/diff/
Testing
-------
make check
Thanks,
Brenden Matthews
Re: Review Request 12885: Slave ping timeout may be increased by flags.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12885/#review24300
-----------------------------------------------------------
Ship it!
src/master/flags.hpp
<https://reviews.apache.org/r/12885/#comment48130>
s/sec/secs/
Yup, our parser is a bit dumb. Feel free to make it better! https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/include/stout/duration.hpp#L42
- Benjamin Hindman
On July 30, 2013, 10:02 p.m., Brenden Matthews wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12885/
> -----------------------------------------------------------
>
> (Updated July 30, 2013, 10:02 p.m.)
>
>
> Review request for mesos.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Slave ping timeout may be increased by flags.
>
> Review: https://reviews.apache.org/r/12885
>
>
> Diffs
> -----
>
> src/master/constants.hpp f3a95f1006472ff2b90cabb8cf74182830740954
> src/master/constants.cpp 7bc32eb3bdae717ded63436e47031934384eec23
> src/master/flags.hpp 6dbbb79d447991a4929fe27628961c0874adadf8
> src/master/master.cpp e4507acd97ebaf2ce693ec7343e9a4a563f6ff80
> src/tests/fault_tolerance_tests.cpp c8d88d5f60cf49ef5c1ffa429d308c30c2e5005c
>
> Diff: https://reviews.apache.org/r/12885/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Brenden Matthews
>
>
Re: Review Request 12885: Slave ping timeout may be increased by flags.
Posted by Brenden Matthews <br...@diddyinc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12885/
-----------------------------------------------------------
(Updated July 30, 2013, 10:02 p.m.)
Review request for mesos.
Repository: mesos-git
Description
-------
Slave ping timeout may be increased by flags.
Review: https://reviews.apache.org/r/12885
Diffs
-----
src/master/constants.hpp f3a95f1006472ff2b90cabb8cf74182830740954
src/master/constants.cpp 7bc32eb3bdae717ded63436e47031934384eec23
src/master/flags.hpp 6dbbb79d447991a4929fe27628961c0874adadf8
src/master/master.cpp e4507acd97ebaf2ce693ec7343e9a4a563f6ff80
src/tests/fault_tolerance_tests.cpp c8d88d5f60cf49ef5c1ffa429d308c30c2e5005c
Diff: https://reviews.apache.org/r/12885/diff/
Testing
-------
make check
Thanks,
Brenden Matthews
Re: Review Request 12885: Slave ping timeout may be increased by flags.
Posted by Brenden Matthews <br...@diddyinc.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12885/
-----------------------------------------------------------
(Updated July 30, 2013, 10:02 p.m.)
Review request for mesos.
Changes
-------
Update in accordance with the Bens' comments.
Repository: mesos-git
Description
-------
Slave ping timeout may be increased by flags.
Review: https://reviews.apache.org/r/12885
Diffs (updated)
-----
src/master/constants.hpp f3a95f1006472ff2b90cabb8cf74182830740954
src/master/constants.cpp 7bc32eb3bdae717ded63436e47031934384eec23
src/master/flags.hpp 6dbbb79d447991a4929fe27628961c0874adadf8
src/master/master.cpp e4507acd97ebaf2ce693ec7343e9a4a563f6ff80
src/tests/fault_tolerance_tests.cpp c8d88d5f60cf49ef5c1ffa429d308c30c2e5005c
Diff: https://reviews.apache.org/r/12885/diff/
Testing
-------
make check
Thanks,
Brenden Matthews
Re: Review Request 12885: Slave ping timeout may be increased by flags.
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On July 30, 2013, 7:47 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 145-151
> > <https://reviews.apache.org/r/12885/diff/1/?file=326194#file326194line145>
> >
> > Instead of overriding implicitly, can you change this to use EXIT(1)?
> >
> > EXIT(1) << "...";
> >
> > It would be better to be explicit about the fact that their flag value is invalid.
>
> Brenden Matthews wrote:
> I would also suggest using EXIT_FAILURE instead of 1/0, but I'll go with the existing convention.
SGTM! Want to create a JIRA issue to replace all EXIT(*) with EXIT(EXIT_FAILURE)? Or maybe better would be an empty EXIT() which uses EXIT_FAILURE?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12885/#review24273
-----------------------------------------------------------
On July 30, 2013, 10:02 p.m., Brenden Matthews wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12885/
> -----------------------------------------------------------
>
> (Updated July 30, 2013, 10:02 p.m.)
>
>
> Review request for mesos.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Slave ping timeout may be increased by flags.
>
> Review: https://reviews.apache.org/r/12885
>
>
> Diffs
> -----
>
> src/master/constants.hpp f3a95f1006472ff2b90cabb8cf74182830740954
> src/master/constants.cpp 7bc32eb3bdae717ded63436e47031934384eec23
> src/master/flags.hpp 6dbbb79d447991a4929fe27628961c0874adadf8
> src/master/master.cpp e4507acd97ebaf2ce693ec7343e9a4a563f6ff80
> src/tests/fault_tolerance_tests.cpp c8d88d5f60cf49ef5c1ffa429d308c30c2e5005c
>
> Diff: https://reviews.apache.org/r/12885/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Brenden Matthews
>
>
Re: Review Request 12885: Slave ping timeout may be increased by flags.
Posted by Brenden Matthews <br...@diddyinc.com>.
> On July 30, 2013, 7:47 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 145-151
> > <https://reviews.apache.org/r/12885/diff/1/?file=326194#file326194line145>
> >
> > Instead of overriding implicitly, can you change this to use EXIT(1)?
> >
> > EXIT(1) << "...";
> >
> > It would be better to be explicit about the fact that their flag value is invalid.
I would also suggest using EXIT_FAILURE instead of 1/0, but I'll go with the existing convention.
- Brenden
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12885/#review24273
-----------------------------------------------------------
On July 23, 2013, 11:42 p.m., Brenden Matthews wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12885/
> -----------------------------------------------------------
>
> (Updated July 23, 2013, 11:42 p.m.)
>
>
> Review request for mesos.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Slave ping timeout may be increased by flags.
>
> Review: https://reviews.apache.org/r/12885
>
>
> Diffs
> -----
>
> src/master/constants.hpp f3a95f1006472ff2b90cabb8cf74182830740954
> src/master/constants.cpp 7bc32eb3bdae717ded63436e47031934384eec23
> src/master/flags.hpp 6dbbb79d447991a4929fe27628961c0874adadf8
> src/master/master.cpp e4507acd97ebaf2ce693ec7343e9a4a563f6ff80
> src/tests/fault_tolerance_tests.cpp c8d88d5f60cf49ef5c1ffa429d308c30c2e5005c
>
> Diff: https://reviews.apache.org/r/12885/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Brenden Matthews
>
>
Re: Review Request 12885: Slave ping timeout may be increased by flags.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12885/#review24273
-----------------------------------------------------------
src/master/flags.hpp
<https://reviews.apache.org/r/12885/#comment48102>
s/" /"/
src/master/flags.hpp
<https://reviews.apache.org/r/12885/#comment48103>
s/" /"/
s/)/)./
src/master/master.cpp
<https://reviews.apache.org/r/12885/#comment48099>
Instead of overriding implicitly, can you change this to use EXIT(1)?
EXIT(1) << "...";
It would be better to be explicit about the fact that their flag value is invalid.
src/master/master.cpp
<https://reviews.apache.org/r/12885/#comment48101>
In Master::deactivateSlave, there's a WARNING for this, any reason to add this additional WARNING?
- Ben Mahler
On July 23, 2013, 11:42 p.m., Brenden Matthews wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12885/
> -----------------------------------------------------------
>
> (Updated July 23, 2013, 11:42 p.m.)
>
>
> Review request for mesos.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Slave ping timeout may be increased by flags.
>
> Review: https://reviews.apache.org/r/12885
>
>
> Diffs
> -----
>
> src/master/constants.hpp f3a95f1006472ff2b90cabb8cf74182830740954
> src/master/constants.cpp 7bc32eb3bdae717ded63436e47031934384eec23
> src/master/flags.hpp 6dbbb79d447991a4929fe27628961c0874adadf8
> src/master/master.cpp e4507acd97ebaf2ce693ec7343e9a4a563f6ff80
> src/tests/fault_tolerance_tests.cpp c8d88d5f60cf49ef5c1ffa429d308c30c2e5005c
>
> Diff: https://reviews.apache.org/r/12885/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Brenden Matthews
>
>
Re: Review Request 12885: Slave ping timeout may be increased by flags.
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12885/#review24278
-----------------------------------------------------------
Ship it!
src/master/flags.hpp
<https://reviews.apache.org/r/12885/#comment48113>
I don't think we parse 's', you need all of 'secs'. Also, please kill the whitespace at the beginning of the continuing lines.
src/master/master.cpp
<https://reviews.apache.org/r/12885/#comment48111>
Let's just pass all of 'flags' in and then read it out like 'flags.slave_ping_timeout'. It's more obvious when doing it like that where this value came from. Also, we put '&' next to the type. If you really don't want to pass flags then please s/slave_ping_timeout/slavePingTimeout/. I know, it's a drag, but we only name our flag variables with underscores to have consistency with the command line arguments. Feel free to take on a complete rename of all variables in Mesos to go from camel case to snake case ... I don't think anyone will fight you!
src/master/master.cpp
<https://reviews.apache.org/r/12885/#comment48110>
Please do this check in Master::initialize and do an EXIT(1) there.
src/master/master.cpp
<https://reviews.apache.org/r/12885/#comment48112>
So, 'flags.slave_ping_timeout'.
- Benjamin Hindman
On July 23, 2013, 11:42 p.m., Brenden Matthews wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12885/
> -----------------------------------------------------------
>
> (Updated July 23, 2013, 11:42 p.m.)
>
>
> Review request for mesos.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Slave ping timeout may be increased by flags.
>
> Review: https://reviews.apache.org/r/12885
>
>
> Diffs
> -----
>
> src/master/constants.hpp f3a95f1006472ff2b90cabb8cf74182830740954
> src/master/constants.cpp 7bc32eb3bdae717ded63436e47031934384eec23
> src/master/flags.hpp 6dbbb79d447991a4929fe27628961c0874adadf8
> src/master/master.cpp e4507acd97ebaf2ce693ec7343e9a4a563f6ff80
> src/tests/fault_tolerance_tests.cpp c8d88d5f60cf49ef5c1ffa429d308c30c2e5005c
>
> Diff: https://reviews.apache.org/r/12885/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Brenden Matthews
>
>