You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2015/10/01 12:07:03 UTC
Re: Review Request 38161: Replaced a hard-coded number for
registration backoff with a proper constant.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38161/
-----------------------------------------------------------
(Updated Oct. 1, 2015, 10:07 a.m.)
Review request for mesos and Ben Mahler.
Changes
-------
BenM's comments.
Summary (updated)
-----------------
Replaced a hard-coded number for registration backoff with a proper constant.
Repository: mesos
Description (updated)
-------
Prepend a constant regulating the backoff factor with DEFAULT_ prefix for clarity, use it in tests where appropriate and extend comments for posterity.
Diffs (updated)
-----
src/sched/constants.hpp ac497b2abc9fbeab713b312cf37963d7de28ed50
src/sched/constants.cpp 517ca5cb0dad49d775d65c9ffabc849ff85347a0
src/sched/flags.hpp 4e0d56f61b6877254161112b288ae4a9c579e846
src/slave/constants.hpp df18676f17f2277e3c38432b76f16c5f9cb08341
src/slave/constants.cpp cf3ee7bbc252364a1b73731feab6a9da68ee1f55
src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862
src/tests/fault_tolerance_tests.cpp c97bc4691f9bac4a8677e6d2247be96ee9674b57
Diff: https://reviews.apache.org/r/38161/diff/
Testing
-------
make check (Mac OS 10.10.4)
Thanks,
Alexander Rukletsov
Re: Review Request 38161: Replaced a hard-coded number for
registration backoff with a proper constant.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Oct. 1, 2015, 6:08 p.m., Ben Mahler wrote:
> > src/tests/fault_tolerance_tests.cpp, line 1540
> > <https://reviews.apache.org/r/38161/diff/4/?file=1088390#file1088390line1540>
> >
> > I could have committed the `> >` flattening already for you if it was split, also, it would be be better to do a pass on the entire file in one patch rather than stick in it alongside the rest of your change, thoughts?
Sure, see https://reviews.apache.org/r/38952/
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38161/#review101250
-----------------------------------------------------------
On Oct. 1, 2015, 10:07 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38161/
> -----------------------------------------------------------
>
> (Updated Oct. 1, 2015, 10:07 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prepend a constant regulating the backoff factor with DEFAULT_ prefix for clarity, use it in tests where appropriate and extend comments for posterity.
>
>
> Diffs
> -----
>
> src/sched/constants.hpp ac497b2abc9fbeab713b312cf37963d7de28ed50
> src/sched/constants.cpp 517ca5cb0dad49d775d65c9ffabc849ff85347a0
> src/sched/flags.hpp 4e0d56f61b6877254161112b288ae4a9c579e846
> src/slave/constants.hpp df18676f17f2277e3c38432b76f16c5f9cb08341
> src/slave/constants.cpp cf3ee7bbc252364a1b73731feab6a9da68ee1f55
> src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862
> src/tests/fault_tolerance_tests.cpp c97bc4691f9bac4a8677e6d2247be96ee9674b57
>
> Diff: https://reviews.apache.org/r/38161/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 38161: Replaced a hard-coded number for
registration backoff with a proper constant.
Posted by Ben Mahler <be...@gmail.com>.
> On Oct. 1, 2015, 6:08 p.m., Ben Mahler wrote:
> > src/tests/fault_tolerance_tests.cpp, lines 1547-1551
> > <https://reviews.apache.org/r/38161/diff/4/?file=1088390#file1088390line1547>
> >
> > It looks inconsistent to do this here but not in the other tests this patch touches. Now that the variable name includes DEFAULT, how about we remove the explicit setting here?
>
> Alexander Rukletsov wrote:
> We do not use this value for `Clock::advance()` in other tests. Though it is indeed inconsistent, explicit setting ensures we advance for the same duration as used by the agent. If we remove it, `CreateSlaveFlags()` may change this value.
Really? I see two other instances of Clock::advance with DEFAULT_REGISTRATION_BACKOFF_FACTOR passed in, looking at the diff alone.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38161/#review101250
-----------------------------------------------------------
On Oct. 2, 2015, 3:13 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38161/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2015, 3:13 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prepend a constant regulating the backoff factor with DEFAULT_ prefix for clarity, use it in tests where appropriate and extend comments for posterity.
>
>
> Diffs
> -----
>
> src/sched/constants.hpp ac497b2abc9fbeab713b312cf37963d7de28ed50
> src/sched/constants.cpp 517ca5cb0dad49d775d65c9ffabc849ff85347a0
> src/sched/flags.hpp 4e0d56f61b6877254161112b288ae4a9c579e846
> src/slave/constants.hpp df18676f17f2277e3c38432b76f16c5f9cb08341
> src/slave/constants.cpp cf3ee7bbc252364a1b73731feab6a9da68ee1f55
> src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862
> src/tests/fault_tolerance_tests.cpp c97bc4691f9bac4a8677e6d2247be96ee9674b57
>
> Diff: https://reviews.apache.org/r/38161/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 38161: Replaced a hard-coded number for
registration backoff with a proper constant.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Oct. 1, 2015, 6:08 p.m., Ben Mahler wrote:
> > src/tests/fault_tolerance_tests.cpp, lines 1547-1551
> > <https://reviews.apache.org/r/38161/diff/4/?file=1088390#file1088390line1547>
> >
> > It looks inconsistent to do this here but not in the other tests this patch touches. Now that the variable name includes DEFAULT, how about we remove the explicit setting here?
>
> Alexander Rukletsov wrote:
> We do not use this value for `Clock::advance()` in other tests. Though it is indeed inconsistent, explicit setting ensures we advance for the same duration as used by the agent. If we remove it, `CreateSlaveFlags()` may change this value.
>
> Ben Mahler wrote:
> Really? I see two other instances of Clock::advance with DEFAULT_REGISTRATION_BACKOFF_FACTOR passed in, looking at the diff alone.
Okay, these two do not count, they are being introduced now : ). The difference between those and this one is that AFAIK we cannot change Scheduler flags from the tests, but we can do it with the agent flags.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38161/#review101250
-----------------------------------------------------------
On Oct. 2, 2015, 3:13 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38161/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2015, 3:13 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prepend a constant regulating the backoff factor with DEFAULT_ prefix for clarity, use it in tests where appropriate and extend comments for posterity.
>
>
> Diffs
> -----
>
> src/sched/constants.hpp ac497b2abc9fbeab713b312cf37963d7de28ed50
> src/sched/constants.cpp 517ca5cb0dad49d775d65c9ffabc849ff85347a0
> src/sched/flags.hpp 4e0d56f61b6877254161112b288ae4a9c579e846
> src/slave/constants.hpp df18676f17f2277e3c38432b76f16c5f9cb08341
> src/slave/constants.cpp cf3ee7bbc252364a1b73731feab6a9da68ee1f55
> src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862
> src/tests/fault_tolerance_tests.cpp c97bc4691f9bac4a8677e6d2247be96ee9674b57
>
> Diff: https://reviews.apache.org/r/38161/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 38161: Replaced a hard-coded number for
registration backoff with a proper constant.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Oct. 1, 2015, 6:08 p.m., Ben Mahler wrote:
> > src/tests/fault_tolerance_tests.cpp, lines 650-651
> > <https://reviews.apache.org/r/38161/diff/4/?file=1088390#file1088390line650>
> >
> > I don't think we need this second sentence, since the DEFAULT expresses this pretty clearly, no?
My personal preference is to be more verbose. I think for a project newbie it's hard to grasp that `MesosSchedulerDriver driver2(&sched2, framework2, master.get(), DEFAULT_CREDENTIAL);` sets a registration backoff to a certain value which we use later on to advance the clock. However, I also see your point => removing the sentence.
> On Oct. 1, 2015, 6:08 p.m., Ben Mahler wrote:
> > src/tests/fault_tolerance_tests.cpp, lines 1547-1551
> > <https://reviews.apache.org/r/38161/diff/4/?file=1088390#file1088390line1547>
> >
> > It looks inconsistent to do this here but not in the other tests this patch touches. Now that the variable name includes DEFAULT, how about we remove the explicit setting here?
We do not use this value for `Clock::advance()` in other tests. Though it is indeed inconsistent, explicit setting ensures we advance for the same duration as used by the agent. If we remove it, `CreateSlaveFlags()` may change this value.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38161/#review101250
-----------------------------------------------------------
On Oct. 1, 2015, 10:07 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38161/
> -----------------------------------------------------------
>
> (Updated Oct. 1, 2015, 10:07 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prepend a constant regulating the backoff factor with DEFAULT_ prefix for clarity, use it in tests where appropriate and extend comments for posterity.
>
>
> Diffs
> -----
>
> src/sched/constants.hpp ac497b2abc9fbeab713b312cf37963d7de28ed50
> src/sched/constants.cpp 517ca5cb0dad49d775d65c9ffabc849ff85347a0
> src/sched/flags.hpp 4e0d56f61b6877254161112b288ae4a9c579e846
> src/slave/constants.hpp df18676f17f2277e3c38432b76f16c5f9cb08341
> src/slave/constants.cpp cf3ee7bbc252364a1b73731feab6a9da68ee1f55
> src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862
> src/tests/fault_tolerance_tests.cpp c97bc4691f9bac4a8677e6d2247be96ee9674b57
>
> Diff: https://reviews.apache.org/r/38161/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 38161: Replaced a hard-coded number for
registration backoff with a proper constant.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38161/#review101250
-----------------------------------------------------------
Thanks for the patience, almost there! Can we split the `> >` flattening change out and do a complete pass on the file?
src/tests/fault_tolerance_tests.cpp (lines 650 - 651)
<https://reviews.apache.org/r/38161/#comment158628>
I don't think we need this second sentence, since the DEFAULT expresses this pretty clearly, no?
src/tests/fault_tolerance_tests.cpp (lines 706 - 707)
<https://reviews.apache.org/r/38161/#comment158627>
Ditto here.
src/tests/fault_tolerance_tests.cpp (line 1540)
<https://reviews.apache.org/r/38161/#comment158625>
I could have committed the `> >` flattening already for you if it was split, also, it would be be better to do a pass on the entire file in one patch rather than stick in it alongside the rest of your change, thoughts?
src/tests/fault_tolerance_tests.cpp (lines 1547 - 1551)
<https://reviews.apache.org/r/38161/#comment158623>
It looks inconsistent to do this here but not in the other tests this patch touches. Now that the variable name includes DEFAULT, how about we remove the explicit setting here?
- Ben Mahler
On Oct. 1, 2015, 10:07 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38161/
> -----------------------------------------------------------
>
> (Updated Oct. 1, 2015, 10:07 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prepend a constant regulating the backoff factor with DEFAULT_ prefix for clarity, use it in tests where appropriate and extend comments for posterity.
>
>
> Diffs
> -----
>
> src/sched/constants.hpp ac497b2abc9fbeab713b312cf37963d7de28ed50
> src/sched/constants.cpp 517ca5cb0dad49d775d65c9ffabc849ff85347a0
> src/sched/flags.hpp 4e0d56f61b6877254161112b288ae4a9c579e846
> src/slave/constants.hpp df18676f17f2277e3c38432b76f16c5f9cb08341
> src/slave/constants.cpp cf3ee7bbc252364a1b73731feab6a9da68ee1f55
> src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862
> src/tests/fault_tolerance_tests.cpp c97bc4691f9bac4a8677e6d2247be96ee9674b57
>
> Diff: https://reviews.apache.org/r/38161/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 38161: Replaced a hard-coded number for
registration backoff with a proper constant.
Posted by Bernd Mathiske <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38161/#review102636
-----------------------------------------------------------
Ship it!
Ship It!
- Bernd Mathiske
On Oct. 14, 2015, 8:51 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38161/
> -----------------------------------------------------------
>
> (Updated Oct. 14, 2015, 8:51 a.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Prepend a constant regulating the backoff factor with DEFAULT_ prefix for clarity, use it in tests where appropriate and extend comments for posterity.
>
>
> Diffs
> -----
>
> src/sched/constants.hpp ac497b2abc9fbeab713b312cf37963d7de28ed50
> src/sched/constants.cpp 517ca5cb0dad49d775d65c9ffabc849ff85347a0
> src/sched/flags.hpp 4e0d56f61b6877254161112b288ae4a9c579e846
> src/slave/constants.hpp 1197268576ee2ec37601db75ea9536ec09882886
> src/slave/constants.cpp 96dadce6d28aa585410f50a2509a34445f8dcf82
> src/slave/flags.cpp ac68e1bc51874c7d3bb7e08ba4b5329e8ece0d0e
> src/tests/fault_tolerance_tests.cpp c97bc4691f9bac4a8677e6d2247be96ee9674b57
>
> Diff: https://reviews.apache.org/r/38161/diff/
>
>
> Testing
> -------
>
> make check (Mac OS 10.10.4)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 38161: Replaced a hard-coded number for
registration backoff with a proper constant.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38161/
-----------------------------------------------------------
(Updated Oct. 14, 2015, 3:51 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
BenM's comment.
Repository: mesos
Description
-------
Prepend a constant regulating the backoff factor with DEFAULT_ prefix for clarity, use it in tests where appropriate and extend comments for posterity.
Diffs (updated)
-----
src/sched/constants.hpp ac497b2abc9fbeab713b312cf37963d7de28ed50
src/sched/constants.cpp 517ca5cb0dad49d775d65c9ffabc849ff85347a0
src/sched/flags.hpp 4e0d56f61b6877254161112b288ae4a9c579e846
src/slave/constants.hpp 1197268576ee2ec37601db75ea9536ec09882886
src/slave/constants.cpp 96dadce6d28aa585410f50a2509a34445f8dcf82
src/slave/flags.cpp ac68e1bc51874c7d3bb7e08ba4b5329e8ece0d0e
src/tests/fault_tolerance_tests.cpp c97bc4691f9bac4a8677e6d2247be96ee9674b57
Diff: https://reviews.apache.org/r/38161/diff/
Testing
-------
make check (Mac OS 10.10.4)
Thanks,
Alexander Rukletsov
Re: Review Request 38161: Replaced a hard-coded number for
registration backoff with a proper constant.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38161/
-----------------------------------------------------------
(Updated Oct. 2, 2015, 3:13 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Rebased.
Repository: mesos
Description
-------
Prepend a constant regulating the backoff factor with DEFAULT_ prefix for clarity, use it in tests where appropriate and extend comments for posterity.
Diffs (updated)
-----
src/sched/constants.hpp ac497b2abc9fbeab713b312cf37963d7de28ed50
src/sched/constants.cpp 517ca5cb0dad49d775d65c9ffabc849ff85347a0
src/sched/flags.hpp 4e0d56f61b6877254161112b288ae4a9c579e846
src/slave/constants.hpp df18676f17f2277e3c38432b76f16c5f9cb08341
src/slave/constants.cpp cf3ee7bbc252364a1b73731feab6a9da68ee1f55
src/slave/flags.cpp 029aa1eb00e1aa3e92c2155925022c17fd905862
src/tests/fault_tolerance_tests.cpp c97bc4691f9bac4a8677e6d2247be96ee9674b57
Diff: https://reviews.apache.org/r/38161/diff/
Testing
-------
make check (Mac OS 10.10.4)
Thanks,
Alexander Rukletsov