You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2015/02/18 02:16:40 UTC
Review Request 31141: Fixed a clock bug in next().
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31141/
-----------------------------------------------------------
Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
Repository: mesos
Description
-------
When the clock is paused, `next()` should return `None()` since no timers can fire until the clock is advanced or updated.
As a regression during the re-factoring, the `Clock::now()` used to schedule these timers can now be called when `__process__` is set! That led to a flaky test when applying the above fix.
Diffs
-----
3rdparty/libprocess/src/clock.cpp b92d5de2631986506361e28c05cb6169e6632a70
Diff: https://reviews.apache.org/r/31141/diff/
Testing
-------
make check
Thanks,
Ben Mahler
Re: Review Request 31141: Fixed a clock bug in next().
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31141/#review72888
-----------------------------------------------------------
Ship it!
Ship It!
- Joris Van Remoortere
On Feb. 18, 2015, 3:11 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31141/
> -----------------------------------------------------------
>
> (Updated Feb. 18, 2015, 3:11 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When the clock is paused, `next()` should return `None()` since no timers can fire until the clock is advanced or updated.
>
> As a regression during the re-factoring, the `Clock::now()` used to schedule these timers can now be called when `__process__` is set! That led to a flaky test when applying the above fix.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/clock.cpp b92d5de2631986506361e28c05cb6169e6632a70
>
> Diff: https://reviews.apache.org/r/31141/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 31141: Fixed a clock bug in next().
Posted by Ben Mahler <be...@gmail.com>.
> On Feb. 18, 2015, 3:17 a.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/src/clock.cpp, line 72
> > <https://reviews.apache.org/r/31141/diff/2/?file=867208#file867208line72>
> >
> > Can we add another comment here, perhaps after we've calcuated 'duration', that explains that it's possible that 'duration' is negative which implies the timer should have expired but that this is an accepted result. And maybe even add a brief comment for as much in the comment above this function? Thanks Ben, I want to leave as many breadcrumbs for the next people that work on this code as possible.
This becomes a bit simpler in the follow up patch as it returns a Time which cannot be negative. I'll add this comment to that review where we compute the duration of the delay off the of time.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31141/#review72886
-----------------------------------------------------------
On Feb. 18, 2015, 3:11 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31141/
> -----------------------------------------------------------
>
> (Updated Feb. 18, 2015, 3:11 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When the clock is paused, `next()` should return `None()` since no timers can fire until the clock is advanced or updated.
>
> As a regression during the re-factoring, the `Clock::now()` used to schedule these timers can now be called when `__process__` is set! That led to a flaky test when applying the above fix.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/clock.cpp b92d5de2631986506361e28c05cb6169e6632a70
>
> Diff: https://reviews.apache.org/r/31141/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 31141: Fixed a clock bug in next().
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31141/#review72886
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/src/clock.cpp
<https://reviews.apache.org/r/31141/#comment119007>
Not your bug but s/'timer's/'timers'/
3rdparty/libprocess/src/clock.cpp
<https://reviews.apache.org/r/31141/#comment119008>
Can we add another comment here, perhaps after we've calcuated 'duration', that explains that it's possible that 'duration' is negative which implies the timer should have expired but that this is an accepted result. And maybe even add a brief comment for as much in the comment above this function? Thanks Ben, I want to leave as many breadcrumbs for the next people that work on this code as possible.
3rdparty/libprocess/src/clock.cpp
<https://reviews.apache.org/r/31141/#comment119009>
Please add a comment just above this that explains we're passing NULL to be explicit about getting the global clock value, thanks Ben!
- Benjamin Hindman
On Feb. 18, 2015, 3:11 a.m., Ben Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31141/
> -----------------------------------------------------------
>
> (Updated Feb. 18, 2015, 3:11 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> When the clock is paused, `next()` should return `None()` since no timers can fire until the clock is advanced or updated.
>
> As a regression during the re-factoring, the `Clock::now()` used to schedule these timers can now be called when `__process__` is set! That led to a flaky test when applying the above fix.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/clock.cpp b92d5de2631986506361e28c05cb6169e6632a70
>
> Diff: https://reviews.apache.org/r/31141/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Ben Mahler
>
>
Re: Review Request 31141: Fixed a clock bug in next().
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31141/
-----------------------------------------------------------
(Updated Feb. 18, 2015, 3:11 a.m.)
Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
Changes
-------
Updated the documentation for next().
Repository: mesos
Description
-------
When the clock is paused, `next()` should return `None()` since no timers can fire until the clock is advanced or updated.
As a regression during the re-factoring, the `Clock::now()` used to schedule these timers can now be called when `__process__` is set! That led to a flaky test when applying the above fix.
Diffs (updated)
-----
3rdparty/libprocess/src/clock.cpp b92d5de2631986506361e28c05cb6169e6632a70
Diff: https://reviews.apache.org/r/31141/diff/
Testing
-------
make check
Thanks,
Ben Mahler