You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Vinod Kone <vi...@gmail.com> on 2015/02/02 19:49:27 UTC
Review Request 30514: Rate limited the removal of slaves failing
health checks.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/
-----------------------------------------------------------
Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
Bugs: MESOS-1148
https://issues.apache.org/jira/browse/MESOS-1148
Repository: mesos
Description
-------
Rrate limited the removal of slaves failing health checks.
Diffs
-----
src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
src/master/master.hpp 337e00aa46ea127f3667e3383d631c3fb8e22f30
src/master/master.cpp 10056861b95ed9453c971787982db7d09f09f323
src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
Diff: https://reviews.apache.org/r/30514/diff/
Testing
-------
make check
Ran the new tests 100 times
Thanks,
Vinod Kone
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review70644
-----------------------------------------------------------
Patch looks great!
Reviews applied: [30511, 30514]
All tests passed.
- Mesos ReviewBot
On Feb. 2, 2015, 6:51 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2015, 6:51 p.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp 337e00aa46ea127f3667e3383d631c3fb8e22f30
> src/master/master.cpp 10056861b95ed9453c971787982db7d09f09f323
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 2, 2015, 8:13 p.m., Jie Yu wrote:
> > src/master/flags.hpp, line 152
> > <https://reviews.apache.org/r/30514/diff/1/?file=843908#file843908line152>
> >
> > The maximum rate? Does it make sense to you to rename the flag name as well?
> >
> > slave_removal_rate_limit?
sgtm. fixed.
> On Feb. 2, 2015, 8:13 p.m., Jie Yu wrote:
> > src/master/master.hpp, lines 575-577
> > <https://reviews.apache.org/r/30514/diff/1/?file=843909#file843909line575>
> >
> > The moment you pass it to the SlaveObserver, the ownership of the pointer is transferred and you cannot access to that anymore.
> >
> > I guess here you really don't have many options but using a raw pointer (and let the slave manages its lifecycle). I guess we did the same thing for Registrar, WhitelistWatcher, Allocator, etc.
was using Owned as Shared with non-const because i remembered we did that before in our code base. will use shared_ptr with a NOTE instead.
> On Feb. 2, 2015, 8:13 p.m., Jie Yu wrote:
> > src/master/master.cpp, lines 185-207
> > <https://reviews.apache.org/r/30514/diff/1/?file=843910#file843910line185>
> >
> > Can we simply the timeout logic and move all the limiter related stuff to `shutdown` function?
> >
> > ```
> > void timeout()
> > {
> > if (pinged && ++timeouts >= XXX) {
> > // Schedule a shutdown of the slave.
> > shutdown();
> > }
> >
> > // NOTE: We keep sending PINGs even if a shutdown has
> > // been scheduled so that we can cancel the shutdown
> > // if the slave becomes reachable again.
> > ping();
> > }
> >
> > void shutdown()
> > {
> > if (limiter.isNone()) {
> > LOG(INFO) << "...";
> >
> > dispatch(master,
> > &Master::shutdownSlave,
> > ...);
> > return;
> > }
> >
> > if (shuttingDown.isSome()) {
> > // A shutdown has been scheduled already.
> > return;
> > }
> >
> > shuttingDown = limiter.get()->acquire()
> > .onAny(defer(self(), &Self::_shutdown, ...);
> > }
> >
> > void _shutdown(const Future<Nothing> future)
> > {
> > if (future.isReady()) {
> > // Shutdown the slave.
> > } else if (future.isFailed()) {
> > // Not expected to happen?
> > } else if (future.isDiscarded()) {
> > // Do nothing here, maybe print a message.
> > }
> >
> > shuttingDown = None();
> > }
> > ```
looks good. thanks.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review70603
-----------------------------------------------------------
On Feb. 2, 2015, 6:51 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2015, 6:51 p.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp 337e00aa46ea127f3667e3383d631c3fb8e22f30
> src/master/master.cpp 10056861b95ed9453c971787982db7d09f09f323
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review70603
-----------------------------------------------------------
src/master/flags.hpp
<https://reviews.apache.org/r/30514/#comment115811>
The maximum rate? Does it make sense to you to rename the flag name as well?
slave_removal_rate_limit?
src/master/master.hpp
<https://reviews.apache.org/r/30514/#comment115816>
The moment you pass it to the SlaveObserver, the ownership of the pointer is transferred and you cannot access to that anymore.
I guess here you really don't have many options but using a raw pointer (and let the slave manages its lifecycle). I guess we did the same thing for Registrar, WhitelistWatcher, Allocator, etc.
src/master/master.cpp
<https://reviews.apache.org/r/30514/#comment115817>
See my above comments. You probably want to use the raw pointer and add a NOTE about the ownership is controlled by the master.
Option<RateLimiter*> _limiter
src/master/master.cpp
<https://reviews.apache.org/r/30514/#comment115849>
Can we simply the timeout logic and move all the limiter related stuff to `shutdown` function?
```
void timeout()
{
if (pinged && ++timeouts >= XXX) {
// Schedule a shutdown of the slave.
shutdown();
}
// NOTE: We keep sending PINGs even if a shutdown has
// been scheduled so that we can cancel the shutdown
// if the slave becomes reachable again.
ping();
}
void shutdown()
{
if (limiter.isNone()) {
LOG(INFO) << "...";
dispatch(master,
&Master::shutdownSlave,
...);
return;
}
if (shuttingDown.isSome()) {
// A shutdown has been scheduled already.
return;
}
shuttingDown = limiter.get()->acquire()
.onAny(defer(self(), &Self::_shutdown, ...);
}
void _shutdown(const Future<Nothing> future)
{
if (future.isReady()) {
// Shutdown the slave.
} else if (future.isFailed()) {
// Not expected to happen?
} else if (future.isDiscarded()) {
// Do nothing here, maybe print a message.
}
shuttingDown = None();
}
```
src/master/master.cpp
<https://reviews.apache.org/r/30514/#comment115842>
Could you add a NOTE about this function:
```
NOTE: This function schedules a shutdown of the slave. The shutdown of a slave is rate limited (MESOS-XXXX). The shutdown can be cancelled if a pong is received before the actually shutdown is called.
```
src/tests/slave_tests.cpp
<https://reviews.apache.org/r/30514/#comment115852>
indent?
src/tests/slave_tests.cpp
<https://reviews.apache.org/r/30514/#comment115851>
indent?
- Jie Yu
On Feb. 2, 2015, 6:51 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2015, 6:51 p.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp 337e00aa46ea127f3667e3383d631c3fb8e22f30
> src/master/master.cpp 10056861b95ed9453c971787982db7d09f09f323
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 3, 2015, 6:16 p.m., Jie Yu wrote:
> > src/master/flags.hpp, line 152
> > <https://reviews.apache.org/r/30514/diff/2/?file=844914#file844914line152>
> >
> > Good point. I am wondering should we introduce a Rate abstraction in stout:
> >
> > ```
> > Rate rate1(3, Minites(3));
> > Rate rate2(10, Hours(1));
> > ```
> > And the RateLimiter can take a Rate instance:
> > ```
> > RateLimiter limiter(rate2);
> > ```
> > And we can add parser for Rate.
I'll add a TODO for now.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review70775
-----------------------------------------------------------
On Feb. 3, 2015, 2:39 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2015, 2:39 a.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp 337e00aa46ea127f3667e3383d631c3fb8e22f30
> src/master/master.cpp 10056861b95ed9453c971787982db7d09f09f323
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review70775
-----------------------------------------------------------
src/master/flags.hpp
<https://reviews.apache.org/r/30514/#comment116200>
Good point. I am wondering should we introduce a Rate abstraction in stout:
```
Rate rate1(3, Minites(3));
Rate rate2(10, Hours(1));
```
And the RateLimiter can take a Rate instance:
```
RateLimiter limiter(rate2);
```
And we can add parser for Rate.
- Jie Yu
On Feb. 3, 2015, 2:39 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2015, 2:39 a.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp 337e00aa46ea127f3667e3383d631c3fb8e22f30
> src/master/master.cpp 10056861b95ed9453c971787982db7d09f09f323
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review70700
-----------------------------------------------------------
Patch looks great!
Reviews applied: [30511, 30514]
All tests passed.
- Mesos ReviewBot
On Feb. 3, 2015, 2:39 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2015, 2:39 a.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp 337e00aa46ea127f3667e3383d631c3fb8e22f30
> src/master/master.cpp 10056861b95ed9453c971787982db7d09f09f323
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 3, 2015, 6:09 p.m., David Robinson wrote:
> > src/master/flags.hpp, line 152
> > <https://reviews.apache.org/r/30514/diff/2/?file=844914#file844914line152>
> >
> > Should this be slaves pre minute? I imagine most clusters in the wild would be relatively small, and the smaller the cluster the slower you'd want removals?
We have typically taken rates (configured via command line flags) in units per second (e.g., framework rate limits). So wanted to be consistent here. Note that this is a double value, so a per minute rate can be convered to per second rate.
> On Feb. 3, 2015, 6:09 p.m., David Robinson wrote:
> > src/master/master.cpp, line 243
> > <https://reviews.apache.org/r/30514/diff/2/?file=844916#file844916line243>
> >
> > s/is/was/ ?
done
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review70768
-----------------------------------------------------------
On Feb. 3, 2015, 2:39 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2015, 2:39 a.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp 337e00aa46ea127f3667e3383d631c3fb8e22f30
> src/master/master.cpp 10056861b95ed9453c971787982db7d09f09f323
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by David Robinson <dr...@twitter.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review70768
-----------------------------------------------------------
src/master/flags.hpp
<https://reviews.apache.org/r/30514/#comment116199>
Should this be slaves pre minute? I imagine most clusters in the wild would be relatively small, and the smaller the cluster the slower you'd want removals?
src/master/master.cpp
<https://reviews.apache.org/r/30514/#comment116173>
s/is/was/ ?
- David Robinson
On Feb. 3, 2015, 2:39 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2015, 2:39 a.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp 337e00aa46ea127f3667e3383d631c3fb8e22f30
> src/master/master.cpp 10056861b95ed9453c971787982db7d09f09f323
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 3, 2015, 6:25 p.m., Jie Yu wrote:
> > src/tests/slave_tests.cpp, line 1232
> > <https://reviews.apache.org/r/30514/diff/2/?file=844918#file844918line1232>
> >
> > Kill process:: prefix by using namespace process?
done
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review70776
-----------------------------------------------------------
On Feb. 3, 2015, 2:39 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2015, 2:39 a.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp 337e00aa46ea127f3667e3383d631c3fb8e22f30
> src/master/master.cpp 10056861b95ed9453c971787982db7d09f09f323
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review70776
-----------------------------------------------------------
Ship it!
src/master/master.cpp
<https://reviews.apache.org/r/30514/#comment116201>
indent. align the `<<`
src/tests/slave_tests.cpp
<https://reviews.apache.org/r/30514/#comment116202>
Kill process:: prefix by using namespace process?
- Jie Yu
On Feb. 3, 2015, 2:39 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2015, 2:39 a.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp 337e00aa46ea127f3667e3383d631c3fb8e22f30
> src/master/master.cpp 10056861b95ed9453c971787982db7d09f09f323
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Jie Yu <yu...@gmail.com>.
> On Feb. 4, 2015, 8:01 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 197-246
> > <https://reviews.apache.org/r/30514/diff/5/?file=847032#file847032line197>
> >
> > It looks like we only log when we're shutting down the slave, when there's no rate limiter?
> >
> > Have you considered having a single shutdown code path? This gets you a single dispatch, and a single log statement and it looks like it's less control flow?
> >
> > ```
> > void shutdown()
> > {
> > if (shuttingDown.isNone()) {
> > Future<Nothing> future = Nothing();
> >
> > if (limiter.isSome()) {
> > LOG(INFO) << "Scheduling shutdown of slave " << slaveId
> > << " due to health check timeout";
> >
> > future = limiter.get()->acquire();
> > }
> >
> > shuttingDown = future.onAny(defer(self(), &Self::_shutdown));
> >
> > ++metrics->slave_shutdowns_scheduled;
> > }
> > }
> >
> > void _shutdown()
> > {
> > CHECK_SOME(shuttingDown);
> >
> > const Future<Nothing>& future = shuttingDown.get();
> >
> > CHECK(!future.isFailed()) << future.failure();
> >
> > if (future.isReady()) {
> > LOG(INFO) << "Shutting down slave " << slaveId
> > << " due to health check timeout";
> >
> > dispatch(master,
> > &Master::shutdownSlave,
> > slaveId,
> > "health check timed out");
> > } else if (future.isDiscarded()) {
> > // Shutdown was canceled.
> > LOG(INFO) << "Canceling shutdown of slave " << slaveId
> > << " since a pong is received!";
> >
> > ++metrics->slave_shutdowns_canceled;
> > }
> >
> > shuttingDown = None();
> > }
> > ```
>
> Jie Yu wrote:
> Having both 'future' and 'shuttingDown' in 'shutdown' method looks confusing to me:) You need to comment about why you need that future. Or, I think having dup code is not a too bad thing in this case as the logic is more clear IMO.
>
> Jie Yu wrote:
> Another option to avoid dup code is:
>
> 1) s/shutdown/scheduleShutdown
> 2) let 'shutdown' be the actual shutdown (sending message)
>
> Ben Mahler wrote:
> Imagine it is s/future/limited/:
>
> ```
> Future<Nothing> limited = Nothing();
>
> if (limiter.isSome()) {
> limited = limiter.get()->acquire();
> }
>
> shuttingDown = limited.onAny(defer(self(), &Self::_shutdown));
> ```
>
> Seems pretty clear? Can you flush out the second option you mentioned in a code snippet? I didn't follow.
>
> Jie Yu wrote:
> Yeah, that reads much better.
>
> Vinod Kone wrote:
> hmm. i actually liked the explicitness of the earlier version. needed less cognitive overhead. but also liked the proposal of always calling "_shutdown()". here's my proposal:
>
> ```
> void shutdown()
> {
> if (shuttingDown.isSome()) {
> // Shutdown is already in progress.
> return;
> }
>
> if (limiter.isSome()) {
> // If a rate limit is specified schedule the shutdown according
> // to the rate limit.
> LOG(INFO) << "Scheduling shutdown of slave " << slaveId
> << " due to health check timeout";
>
> shuttingDown = limiter.get()->acquire();
> } else {
> // If no rate limit is specified shutdown right away.
> shuttingDown = Future<Nothing>(Nothing()); // Ready future.
> }
>
> shuttingDown.get()
> .onAny(defer(self(), &Self::_shutdown));
> }
>
> ```
>
> what do you guys think?
I like the idea, but this won't compile since shuttingDown.get() returns a const ref and onAny is a non-const function ;-)
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review71021
-----------------------------------------------------------
On Feb. 4, 2015, 1:51 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2015, 1:51 a.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp cd37ee9d3c57bcd91f08cd402ec1e4a09d9e42ee
> src/master/master.cpp d04b2c4041d8fe8978b877f07579a6f907903e1b
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 4, 2015, 8:01 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 197-246
> > <https://reviews.apache.org/r/30514/diff/5/?file=847032#file847032line197>
> >
> > It looks like we only log when we're shutting down the slave, when there's no rate limiter?
> >
> > Have you considered having a single shutdown code path? This gets you a single dispatch, and a single log statement and it looks like it's less control flow?
> >
> > ```
> > void shutdown()
> > {
> > if (shuttingDown.isNone()) {
> > Future<Nothing> future = Nothing();
> >
> > if (limiter.isSome()) {
> > LOG(INFO) << "Scheduling shutdown of slave " << slaveId
> > << " due to health check timeout";
> >
> > future = limiter.get()->acquire();
> > }
> >
> > shuttingDown = future.onAny(defer(self(), &Self::_shutdown));
> >
> > ++metrics->slave_shutdowns_scheduled;
> > }
> > }
> >
> > void _shutdown()
> > {
> > CHECK_SOME(shuttingDown);
> >
> > const Future<Nothing>& future = shuttingDown.get();
> >
> > CHECK(!future.isFailed()) << future.failure();
> >
> > if (future.isReady()) {
> > LOG(INFO) << "Shutting down slave " << slaveId
> > << " due to health check timeout";
> >
> > dispatch(master,
> > &Master::shutdownSlave,
> > slaveId,
> > "health check timed out");
> > } else if (future.isDiscarded()) {
> > // Shutdown was canceled.
> > LOG(INFO) << "Canceling shutdown of slave " << slaveId
> > << " since a pong is received!";
> >
> > ++metrics->slave_shutdowns_canceled;
> > }
> >
> > shuttingDown = None();
> > }
> > ```
>
> Jie Yu wrote:
> Having both 'future' and 'shuttingDown' in 'shutdown' method looks confusing to me:) You need to comment about why you need that future. Or, I think having dup code is not a too bad thing in this case as the logic is more clear IMO.
>
> Jie Yu wrote:
> Another option to avoid dup code is:
>
> 1) s/shutdown/scheduleShutdown
> 2) let 'shutdown' be the actual shutdown (sending message)
>
> Ben Mahler wrote:
> Imagine it is s/future/limited/:
>
> ```
> Future<Nothing> limited = Nothing();
>
> if (limiter.isSome()) {
> limited = limiter.get()->acquire();
> }
>
> shuttingDown = limited.onAny(defer(self(), &Self::_shutdown));
> ```
>
> Seems pretty clear? Can you flush out the second option you mentioned in a code snippet? I didn't follow.
>
> Jie Yu wrote:
> Yeah, that reads much better.
>
> Vinod Kone wrote:
> hmm. i actually liked the explicitness of the earlier version. needed less cognitive overhead. but also liked the proposal of always calling "_shutdown()". here's my proposal:
>
> ```
> void shutdown()
> {
> if (shuttingDown.isSome()) {
> // Shutdown is already in progress.
> return;
> }
>
> if (limiter.isSome()) {
> // If a rate limit is specified schedule the shutdown according
> // to the rate limit.
> LOG(INFO) << "Scheduling shutdown of slave " << slaveId
> << " due to health check timeout";
>
> shuttingDown = limiter.get()->acquire();
> } else {
> // If no rate limit is specified shutdown right away.
> shuttingDown = Future<Nothing>(Nothing()); // Ready future.
> }
>
> shuttingDown.get()
> .onAny(defer(self(), &Self::_shutdown));
> }
>
> ```
>
> what do you guys think?
>
> Jie Yu wrote:
> I like the idea, but this won't compile since shuttingDown.get() returns a const ref and onAny is a non-const function ;-)
it did compile because onAny is a const function. though not sure why it is const!?
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review71021
-----------------------------------------------------------
On Feb. 4, 2015, 1:51 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2015, 1:51 a.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp cd37ee9d3c57bcd91f08cd402ec1e4a09d9e42ee
> src/master/master.cpp d04b2c4041d8fe8978b877f07579a6f907903e1b
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Jie Yu <yu...@gmail.com>.
> On Feb. 4, 2015, 8:01 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 197-246
> > <https://reviews.apache.org/r/30514/diff/5/?file=847032#file847032line197>
> >
> > It looks like we only log when we're shutting down the slave, when there's no rate limiter?
> >
> > Have you considered having a single shutdown code path? This gets you a single dispatch, and a single log statement and it looks like it's less control flow?
> >
> > ```
> > void shutdown()
> > {
> > if (shuttingDown.isNone()) {
> > Future<Nothing> future = Nothing();
> >
> > if (limiter.isSome()) {
> > LOG(INFO) << "Scheduling shutdown of slave " << slaveId
> > << " due to health check timeout";
> >
> > future = limiter.get()->acquire();
> > }
> >
> > shuttingDown = future.onAny(defer(self(), &Self::_shutdown));
> >
> > ++metrics->slave_shutdowns_scheduled;
> > }
> > }
> >
> > void _shutdown()
> > {
> > CHECK_SOME(shuttingDown);
> >
> > const Future<Nothing>& future = shuttingDown.get();
> >
> > CHECK(!future.isFailed()) << future.failure();
> >
> > if (future.isReady()) {
> > LOG(INFO) << "Shutting down slave " << slaveId
> > << " due to health check timeout";
> >
> > dispatch(master,
> > &Master::shutdownSlave,
> > slaveId,
> > "health check timed out");
> > } else if (future.isDiscarded()) {
> > // Shutdown was canceled.
> > LOG(INFO) << "Canceling shutdown of slave " << slaveId
> > << " since a pong is received!";
> >
> > ++metrics->slave_shutdowns_canceled;
> > }
> >
> > shuttingDown = None();
> > }
> > ```
Having both 'future' and 'shuttingDown' in 'shutdown' method looks confusing to me:) You need to comment about why you need that future. Or, I think having dup code is not a too bad thing in this case as the logic is more clear IMO.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review71021
-----------------------------------------------------------
On Feb. 4, 2015, 1:51 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2015, 1:51 a.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp cd37ee9d3c57bcd91f08cd402ec1e4a09d9e42ee
> src/master/master.cpp d04b2c4041d8fe8978b877f07579a6f907903e1b
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 4, 2015, 8:01 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 197-246
> > <https://reviews.apache.org/r/30514/diff/5/?file=847032#file847032line197>
> >
> > It looks like we only log when we're shutting down the slave, when there's no rate limiter?
> >
> > Have you considered having a single shutdown code path? This gets you a single dispatch, and a single log statement and it looks like it's less control flow?
> >
> > ```
> > void shutdown()
> > {
> > if (shuttingDown.isNone()) {
> > Future<Nothing> future = Nothing();
> >
> > if (limiter.isSome()) {
> > LOG(INFO) << "Scheduling shutdown of slave " << slaveId
> > << " due to health check timeout";
> >
> > future = limiter.get()->acquire();
> > }
> >
> > shuttingDown = future.onAny(defer(self(), &Self::_shutdown));
> >
> > ++metrics->slave_shutdowns_scheduled;
> > }
> > }
> >
> > void _shutdown()
> > {
> > CHECK_SOME(shuttingDown);
> >
> > const Future<Nothing>& future = shuttingDown.get();
> >
> > CHECK(!future.isFailed()) << future.failure();
> >
> > if (future.isReady()) {
> > LOG(INFO) << "Shutting down slave " << slaveId
> > << " due to health check timeout";
> >
> > dispatch(master,
> > &Master::shutdownSlave,
> > slaveId,
> > "health check timed out");
> > } else if (future.isDiscarded()) {
> > // Shutdown was canceled.
> > LOG(INFO) << "Canceling shutdown of slave " << slaveId
> > << " since a pong is received!";
> >
> > ++metrics->slave_shutdowns_canceled;
> > }
> >
> > shuttingDown = None();
> > }
> > ```
>
> Jie Yu wrote:
> Having both 'future' and 'shuttingDown' in 'shutdown' method looks confusing to me:) You need to comment about why you need that future. Or, I think having dup code is not a too bad thing in this case as the logic is more clear IMO.
>
> Jie Yu wrote:
> Another option to avoid dup code is:
>
> 1) s/shutdown/scheduleShutdown
> 2) let 'shutdown' be the actual shutdown (sending message)
>
> Ben Mahler wrote:
> Imagine it is s/future/limited/:
>
> ```
> Future<Nothing> limited = Nothing();
>
> if (limiter.isSome()) {
> limited = limiter.get()->acquire();
> }
>
> shuttingDown = limited.onAny(defer(self(), &Self::_shutdown));
> ```
>
> Seems pretty clear? Can you flush out the second option you mentioned in a code snippet? I didn't follow.
>
> Jie Yu wrote:
> Yeah, that reads much better.
hmm. i actually liked the explicitness of the earlier version. needed less cognitive overhead. but also liked the proposal of always calling "_shutdown()". here's my proposal:
```
void shutdown()
{
if (shuttingDown.isSome()) {
// Shutdown is already in progress.
return;
}
if (limiter.isSome()) {
// If a rate limit is specified schedule the shutdown according
// to the rate limit.
LOG(INFO) << "Scheduling shutdown of slave " << slaveId
<< " due to health check timeout";
shuttingDown = limiter.get()->acquire();
} else {
// If no rate limit is specified shutdown right away.
shuttingDown = Future<Nothing>(Nothing()); // Ready future.
}
shuttingDown.get()
.onAny(defer(self(), &Self::_shutdown));
}
```
what do you guys think?
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review71021
-----------------------------------------------------------
On Feb. 4, 2015, 1:51 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2015, 1:51 a.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp cd37ee9d3c57bcd91f08cd402ec1e4a09d9e42ee
> src/master/master.cpp d04b2c4041d8fe8978b877f07579a6f907903e1b
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Ben Mahler <be...@gmail.com>.
> On Feb. 4, 2015, 8:01 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 197-246
> > <https://reviews.apache.org/r/30514/diff/5/?file=847032#file847032line197>
> >
> > It looks like we only log when we're shutting down the slave, when there's no rate limiter?
> >
> > Have you considered having a single shutdown code path? This gets you a single dispatch, and a single log statement and it looks like it's less control flow?
> >
> > ```
> > void shutdown()
> > {
> > if (shuttingDown.isNone()) {
> > Future<Nothing> future = Nothing();
> >
> > if (limiter.isSome()) {
> > LOG(INFO) << "Scheduling shutdown of slave " << slaveId
> > << " due to health check timeout";
> >
> > future = limiter.get()->acquire();
> > }
> >
> > shuttingDown = future.onAny(defer(self(), &Self::_shutdown));
> >
> > ++metrics->slave_shutdowns_scheduled;
> > }
> > }
> >
> > void _shutdown()
> > {
> > CHECK_SOME(shuttingDown);
> >
> > const Future<Nothing>& future = shuttingDown.get();
> >
> > CHECK(!future.isFailed()) << future.failure();
> >
> > if (future.isReady()) {
> > LOG(INFO) << "Shutting down slave " << slaveId
> > << " due to health check timeout";
> >
> > dispatch(master,
> > &Master::shutdownSlave,
> > slaveId,
> > "health check timed out");
> > } else if (future.isDiscarded()) {
> > // Shutdown was canceled.
> > LOG(INFO) << "Canceling shutdown of slave " << slaveId
> > << " since a pong is received!";
> >
> > ++metrics->slave_shutdowns_canceled;
> > }
> >
> > shuttingDown = None();
> > }
> > ```
>
> Jie Yu wrote:
> Having both 'future' and 'shuttingDown' in 'shutdown' method looks confusing to me:) You need to comment about why you need that future. Or, I think having dup code is not a too bad thing in this case as the logic is more clear IMO.
>
> Jie Yu wrote:
> Another option to avoid dup code is:
>
> 1) s/shutdown/scheduleShutdown
> 2) let 'shutdown' be the actual shutdown (sending message)
Imagine it is s/future/limited/:
```
Future<Nothing> limited = Nothing();
if (limiter.isSome()) {
limited = limiter.get()->acquire();
}
shuttingDown = limited.onAny(defer(self(), &Self::_shutdown));
```
Seems pretty clear? Can you flush out the second option you mentioned in a code snippet? I didn't follow.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review71021
-----------------------------------------------------------
On Feb. 4, 2015, 1:51 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2015, 1:51 a.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp cd37ee9d3c57bcd91f08cd402ec1e4a09d9e42ee
> src/master/master.cpp d04b2c4041d8fe8978b877f07579a6f907903e1b
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Jie Yu <yu...@gmail.com>.
> On Feb. 4, 2015, 8:01 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 197-246
> > <https://reviews.apache.org/r/30514/diff/5/?file=847032#file847032line197>
> >
> > It looks like we only log when we're shutting down the slave, when there's no rate limiter?
> >
> > Have you considered having a single shutdown code path? This gets you a single dispatch, and a single log statement and it looks like it's less control flow?
> >
> > ```
> > void shutdown()
> > {
> > if (shuttingDown.isNone()) {
> > Future<Nothing> future = Nothing();
> >
> > if (limiter.isSome()) {
> > LOG(INFO) << "Scheduling shutdown of slave " << slaveId
> > << " due to health check timeout";
> >
> > future = limiter.get()->acquire();
> > }
> >
> > shuttingDown = future.onAny(defer(self(), &Self::_shutdown));
> >
> > ++metrics->slave_shutdowns_scheduled;
> > }
> > }
> >
> > void _shutdown()
> > {
> > CHECK_SOME(shuttingDown);
> >
> > const Future<Nothing>& future = shuttingDown.get();
> >
> > CHECK(!future.isFailed()) << future.failure();
> >
> > if (future.isReady()) {
> > LOG(INFO) << "Shutting down slave " << slaveId
> > << " due to health check timeout";
> >
> > dispatch(master,
> > &Master::shutdownSlave,
> > slaveId,
> > "health check timed out");
> > } else if (future.isDiscarded()) {
> > // Shutdown was canceled.
> > LOG(INFO) << "Canceling shutdown of slave " << slaveId
> > << " since a pong is received!";
> >
> > ++metrics->slave_shutdowns_canceled;
> > }
> >
> > shuttingDown = None();
> > }
> > ```
>
> Jie Yu wrote:
> Having both 'future' and 'shuttingDown' in 'shutdown' method looks confusing to me:) You need to comment about why you need that future. Or, I think having dup code is not a too bad thing in this case as the logic is more clear IMO.
>
> Jie Yu wrote:
> Another option to avoid dup code is:
>
> 1) s/shutdown/scheduleShutdown
> 2) let 'shutdown' be the actual shutdown (sending message)
>
> Ben Mahler wrote:
> Imagine it is s/future/limited/:
>
> ```
> Future<Nothing> limited = Nothing();
>
> if (limiter.isSome()) {
> limited = limiter.get()->acquire();
> }
>
> shuttingDown = limited.onAny(defer(self(), &Self::_shutdown));
> ```
>
> Seems pretty clear? Can you flush out the second option you mentioned in a code snippet? I didn't follow.
Yeah, that reads much better.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review71021
-----------------------------------------------------------
On Feb. 4, 2015, 1:51 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2015, 1:51 a.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp cd37ee9d3c57bcd91f08cd402ec1e4a09d9e42ee
> src/master/master.cpp d04b2c4041d8fe8978b877f07579a6f907903e1b
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Jie Yu <yu...@gmail.com>.
> On Feb. 4, 2015, 8:01 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 197-246
> > <https://reviews.apache.org/r/30514/diff/5/?file=847032#file847032line197>
> >
> > It looks like we only log when we're shutting down the slave, when there's no rate limiter?
> >
> > Have you considered having a single shutdown code path? This gets you a single dispatch, and a single log statement and it looks like it's less control flow?
> >
> > ```
> > void shutdown()
> > {
> > if (shuttingDown.isNone()) {
> > Future<Nothing> future = Nothing();
> >
> > if (limiter.isSome()) {
> > LOG(INFO) << "Scheduling shutdown of slave " << slaveId
> > << " due to health check timeout";
> >
> > future = limiter.get()->acquire();
> > }
> >
> > shuttingDown = future.onAny(defer(self(), &Self::_shutdown));
> >
> > ++metrics->slave_shutdowns_scheduled;
> > }
> > }
> >
> > void _shutdown()
> > {
> > CHECK_SOME(shuttingDown);
> >
> > const Future<Nothing>& future = shuttingDown.get();
> >
> > CHECK(!future.isFailed()) << future.failure();
> >
> > if (future.isReady()) {
> > LOG(INFO) << "Shutting down slave " << slaveId
> > << " due to health check timeout";
> >
> > dispatch(master,
> > &Master::shutdownSlave,
> > slaveId,
> > "health check timed out");
> > } else if (future.isDiscarded()) {
> > // Shutdown was canceled.
> > LOG(INFO) << "Canceling shutdown of slave " << slaveId
> > << " since a pong is received!";
> >
> > ++metrics->slave_shutdowns_canceled;
> > }
> >
> > shuttingDown = None();
> > }
> > ```
>
> Jie Yu wrote:
> Having both 'future' and 'shuttingDown' in 'shutdown' method looks confusing to me:) You need to comment about why you need that future. Or, I think having dup code is not a too bad thing in this case as the logic is more clear IMO.
Another option to avoid dup code is:
1) s/shutdown/scheduleShutdown
2) let 'shutdown' be the actual shutdown (sending message)
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review71021
-----------------------------------------------------------
On Feb. 4, 2015, 1:51 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2015, 1:51 a.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp cd37ee9d3c57bcd91f08cd402ec1e4a09d9e42ee
> src/master/master.cpp d04b2c4041d8fe8978b877f07579a6f907903e1b
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review71021
-----------------------------------------------------------
Very nice! First high level pass, haven't looked at the tests yet.
src/master/master.cpp
<https://reviews.apache.org/r/30514/#comment116535>
Short-circuiting with a pre-increment seems pretty tricky, how about:
```
if (pinged) {
// No pong was received within the timeout!
timeouts++;
if (timeouts >= MAX_SLAVE_PING_TIMEOUTS) {
shutdown();
}
}
```
src/master/master.cpp
<https://reviews.apache.org/r/30514/#comment116545>
It looks like we only log when we're shutting down the slave, when there's no rate limiter?
Have you considered having a single shutdown code path? This gets you a single dispatch, and a single log statement and it looks like it's less control flow?
```
void shutdown()
{
if (shuttingDown.isNone()) {
Future<Nothing> future = Nothing();
if (limiter.isSome()) {
LOG(INFO) << "Scheduling shutdown of slave " << slaveId
<< " due to health check timeout";
future = limiter.get()->acquire();
}
shuttingDown = future.onAny(defer(self(), &Self::_shutdown));
++metrics->slave_shutdowns_scheduled;
}
}
void _shutdown()
{
CHECK_SOME(shuttingDown);
const Future<Nothing>& future = shuttingDown.get();
CHECK(!future.isFailed()) << future.failure();
if (future.isReady()) {
LOG(INFO) << "Shutting down slave " << slaveId
<< " due to health check timeout";
dispatch(master,
&Master::shutdownSlave,
slaveId,
"health check timed out");
} else if (future.isDiscarded()) {
// Shutdown was canceled.
LOG(INFO) << "Canceling shutdown of slave " << slaveId
<< " since a pong is received!";
++metrics->slave_shutdowns_canceled;
}
shuttingDown = None();
}
```
src/master/master.cpp
<https://reviews.apache.org/r/30514/#comment116548>
Whoops, indentation?
- Ben Mahler
On Feb. 4, 2015, 1:51 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2015, 1:51 a.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp cd37ee9d3c57bcd91f08cd402ec1e4a09d9e42ee
> src/master/master.cpp d04b2c4041d8fe8978b877f07579a6f907903e1b
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review71098
-----------------------------------------------------------
Ship it!
Ship It!
- Ben Mahler
On Feb. 5, 2015, 12:13 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 5, 2015, 12:13 a.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp cd37ee9d3c57bcd91f08cd402ec1e4a09d9e42ee
> src/master/master.cpp 69b945df58c173322b95aeae9cf4ed70329c4bb3
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/
-----------------------------------------------------------
(Updated Feb. 5, 2015, 12:13 a.m.)
Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
Changes
-------
comments.
Bugs: MESOS-1148
https://issues.apache.org/jira/browse/MESOS-1148
Repository: mesos
Description
-------
The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
Diffs (updated)
-----
src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
src/master/master.hpp cd37ee9d3c57bcd91f08cd402ec1e4a09d9e42ee
src/master/master.cpp 69b945df58c173322b95aeae9cf4ed70329c4bb3
src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
Diff: https://reviews.apache.org/r/30514/diff/
Testing
-------
make check
Ran the new tests 100 times
Thanks,
Vinod Kone
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 4, 2015, 11:01 p.m., Ben Mahler wrote:
> > src/master/master.cpp, line 243
> > <https://reviews.apache.org/r/30514/diff/6/?file=849030#file849030line243>
> >
> > Does this comment add any insight? It looks like the LOG(INFO) line reveals this already :)
> >
> > Also, it looks like you're sometimes using two l's and sometimes using one l :)
yea this comment was added before I moved the LOG here. killed it. The 'l's were fixed in the subsequent review, will pull them here.
> On Feb. 4, 2015, 11:01 p.m., Ben Mahler wrote:
> > src/tests/slave_tests.cpp, lines 1336-1345
> > <https://reviews.apache.org/r/30514/diff/6/?file=849032#file849032line1336>
> >
> > Looks good. Do you think we want to inject a rate limiter at some point?
yea, was thinking about that. will add a TODO.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review71059
-----------------------------------------------------------
On Feb. 4, 2015, 10:43 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2015, 10:43 p.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp cd37ee9d3c57bcd91f08cd402ec1e4a09d9e42ee
> src/master/master.cpp 69b945df58c173322b95aeae9cf4ed70329c4bb3
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Jie Yu <yu...@gmail.com>.
> On Feb. 4, 2015, 11:01 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 210-223
> > <https://reviews.apache.org/r/30514/diff/6/?file=849030#file849030line210>
> >
> > The .get() seems a bit hacky to me (as Jie said it's not really const), it seems nice to set `shuttingDown` once at the end of the method to the correct future.
> >
> > The Future<Nothing>(Nothing()) also looks a bit cluttered?
> >
> > This was the motivation for obtaining the 'limited' or 'throttle' future first:
> >
> > ```
> > Future<Nothing> acquire = Nothing();
> >
> > if (limiter.isSome()) {
> > acquire = limiter.get()->acquire();
> > }
> >
> > shuttingDown = acquire.onAny(defer(self(), &Self::_shutdown));
> > ```
> >
> > Also, I'm not sure what the comments are adding over what's in the code? Thoughts?
If the LOG is not a concern, another option is:
```
shuttingDown = (limiter.isSome() ? limiter.get()->acquire() : Nothing())
.onAny(defer(self(), &Self::_shutdown));
```
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review71059
-----------------------------------------------------------
On Feb. 4, 2015, 10:43 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2015, 10:43 p.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp cd37ee9d3c57bcd91f08cd402ec1e4a09d9e42ee
> src/master/master.cpp 69b945df58c173322b95aeae9cf4ed70329c4bb3
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review71059
-----------------------------------------------------------
Took a look at the tests as well.
src/master/master.cpp
<https://reviews.apache.org/r/30514/#comment116594>
Having this seems good, want to move the comment?
```
if (shuttingDown.isSome()) {
return; // Shutdown is already in progress.
}
```
src/master/master.cpp
<https://reviews.apache.org/r/30514/#comment116600>
The .get() seems a bit hacky to me (as Jie said it's not really const), it seems nice to set `shuttingDown` once at the end of the method to the correct future.
The Future<Nothing>(Nothing()) also looks a bit cluttered?
This was the motivation for obtaining the 'limited' or 'throttle' future first:
```
Future<Nothing> acquire = Nothing();
if (limiter.isSome()) {
acquire = limiter.get()->acquire();
}
shuttingDown = acquire.onAny(defer(self(), &Self::_shutdown));
```
Also, I'm not sure what the comments are adding over what's in the code? Thoughts?
src/master/master.cpp
<https://reviews.apache.org/r/30514/#comment116601>
Does this comment add any insight? It looks like the LOG(INFO) line reveals this already :)
Also, it looks like you're sometimes using two l's and sometimes using one l :)
src/tests/slave_tests.cpp
<https://reviews.apache.org/r/30514/#comment116605>
Looks good. Do you think we want to inject a rate limiter at some point?
- Ben Mahler
On Feb. 4, 2015, 10:43 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 4, 2015, 10:43 p.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp cd37ee9d3c57bcd91f08cd402ec1e4a09d9e42ee
> src/master/master.cpp 69b945df58c173322b95aeae9cf4ed70329c4bb3
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/
-----------------------------------------------------------
(Updated Feb. 4, 2015, 10:43 p.m.)
Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
Changes
-------
comments.
Bugs: MESOS-1148
https://issues.apache.org/jira/browse/MESOS-1148
Repository: mesos
Description
-------
The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
Diffs (updated)
-----
src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
src/master/master.hpp cd37ee9d3c57bcd91f08cd402ec1e4a09d9e42ee
src/master/master.cpp 69b945df58c173322b95aeae9cf4ed70329c4bb3
src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
Diff: https://reviews.apache.org/r/30514/diff/
Testing
-------
make check
Ran the new tests 100 times
Thanks,
Vinod Kone
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/
-----------------------------------------------------------
(Updated Feb. 4, 2015, 1:51 a.m.)
Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
Changes
-------
changed the format of the flag to be future proof. it now takes #slaves/duration.
Bugs: MESOS-1148
https://issues.apache.org/jira/browse/MESOS-1148
Repository: mesos
Description
-------
The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
Diffs (updated)
-----
src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
src/master/master.hpp cd37ee9d3c57bcd91f08cd402ec1e4a09d9e42ee
src/master/master.cpp d04b2c4041d8fe8978b877f07579a6f907903e1b
src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
Diff: https://reviews.apache.org/r/30514/diff/
Testing
-------
make check
Ran the new tests 100 times
Thanks,
Vinod Kone
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/
-----------------------------------------------------------
(Updated Feb. 4, 2015, 12:31 a.m.)
Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
Changes
-------
rebased.
Bugs: MESOS-1148
https://issues.apache.org/jira/browse/MESOS-1148
Repository: mesos
Description
-------
The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
Diffs (updated)
-----
src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
src/master/master.hpp cd37ee9d3c57bcd91f08cd402ec1e4a09d9e42ee
src/master/master.cpp d04b2c4041d8fe8978b877f07579a6f907903e1b
src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
Diff: https://reviews.apache.org/r/30514/diff/
Testing
-------
make check
Ran the new tests 100 times
Thanks,
Vinod Kone
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review70804
-----------------------------------------------------------
Patch looks great!
Reviews applied: [30511, 30514]
All tests passed.
- Mesos ReviewBot
On Feb. 3, 2015, 6:42 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2015, 6:42 p.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp 337e00aa46ea127f3667e3383d631c3fb8e22f30
> src/master/master.cpp 10056861b95ed9453c971787982db7d09f09f323
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/
-----------------------------------------------------------
(Updated Feb. 3, 2015, 6:42 p.m.)
Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
Changes
-------
drob's and jie's. NNFR.
Bugs: MESOS-1148
https://issues.apache.org/jira/browse/MESOS-1148
Repository: mesos
Description
-------
The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
Diffs (updated)
-----
src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
src/master/master.hpp 337e00aa46ea127f3667e3383d631c3fb8e22f30
src/master/master.cpp 10056861b95ed9453c971787982db7d09f09f323
src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
Diff: https://reviews.apache.org/r/30514/diff/
Testing
-------
make check
Ran the new tests 100 times
Thanks,
Vinod Kone
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/
-----------------------------------------------------------
(Updated Feb. 3, 2015, 2:39 a.m.)
Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
Changes
-------
dominic's and jie's comments.
Bugs: MESOS-1148
https://issues.apache.org/jira/browse/MESOS-1148
Repository: mesos
Description
-------
The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
Diffs (updated)
-----
src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
src/master/master.hpp 337e00aa46ea127f3667e3383d631c3fb8e22f30
src/master/master.cpp 10056861b95ed9453c971787982db7d09f09f323
src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
Diff: https://reviews.apache.org/r/30514/diff/
Testing
-------
make check
Ran the new tests 100 times
Thanks,
Vinod Kone
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 2, 2015, 6:55 p.m., Dominic Hamon wrote:
> > src/master/master.cpp, line 155
> > <https://reviews.apache.org/r/30514/diff/1/?file=843910#file843910line155>
> >
> > aren't we going towards 0.22.0 now? should this change be in the sprint?
adamb is going to tackle this.
> On Feb. 2, 2015, 6:55 p.m., Dominic Hamon wrote:
> > src/master/master.hpp, line 577
> > <https://reviews.apache.org/r/30514/diff/1/?file=843909#file843909line577>
> >
> > unique_ptr?
> >
> > (yes, i'm going to keep suggesting it)
it's actually a shared_ptr. fixed.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review70602
-----------------------------------------------------------
On Feb. 2, 2015, 6:51 p.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2015, 6:51 p.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp 337e00aa46ea127f3667e3383d631c3fb8e22f30
> src/master/master.cpp 10056861b95ed9453c971787982db7d09f09f323
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/#review70602
-----------------------------------------------------------
src/master/master.hpp
<https://reviews.apache.org/r/30514/#comment115809>
nit: s/use/used/
src/master/master.hpp
<https://reviews.apache.org/r/30514/#comment115810>
unique_ptr?
(yes, i'm going to keep suggesting it)
src/master/master.cpp
<https://reviews.apache.org/r/30514/#comment115812>
aren't we going towards 0.22.0 now? should this change be in the sprint?
src/master/master.cpp
<https://reviews.apache.org/r/30514/#comment115814>
worth logging the received pong?
src/master/master.cpp
<https://reviews.apache.org/r/30514/#comment115815>
i wonder if this should be non-optional but set to a very fast default rate. The logic will be much simpler to follow.
- Dominic Hamon
On Feb. 2, 2015, 10:51 a.m., Vinod Kone wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30514/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2015, 10:51 a.m.)
>
>
> Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
>
>
> Bugs: MESOS-1148
> https://issues.apache.org/jira/browse/MESOS-1148
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
>
>
> Diffs
> -----
>
> src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
> src/master/master.hpp 337e00aa46ea127f3667e3383d631c3fb8e22f30
> src/master/master.cpp 10056861b95ed9453c971787982db7d09f09f323
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
>
> Diff: https://reviews.apache.org/r/30514/diff/
>
>
> Testing
> -------
>
> make check
>
> Ran the new tests 100 times
>
>
> Thanks,
>
> Vinod Kone
>
>
Re: Review Request 30514: Rate limited the removal of slaves failing
health checks.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30514/
-----------------------------------------------------------
(Updated Feb. 2, 2015, 6:51 p.m.)
Review request for mesos, Ben Mahler, David Robinson, and Jie Yu.
Changes
-------
updated description.
Bugs: MESOS-1148
https://issues.apache.org/jira/browse/MESOS-1148
Repository: mesos
Description (updated)
-------
The algorithm is simple. All the slave observers share a rate limiter whose rate is configured via command line. When a slave times out on health check, a permit is acquired to shutdown the slave *but* the pings are continued to be sent. If a pong arrives before the permit is satisifed, the shutdown is cancelled.
Diffs
-----
src/master/flags.hpp 6c18a1af625311ef149b5877b08f63c2b12c040d
src/master/master.hpp 337e00aa46ea127f3667e3383d631c3fb8e22f30
src/master/master.cpp 10056861b95ed9453c971787982db7d09f09f323
src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
src/tests/slave_tests.cpp e7e2af63da785644f3f7e6e23607c02be962a2c6
Diff: https://reviews.apache.org/r/30514/diff/
Testing
-------
make check
Ran the new tests 100 times
Thanks,
Vinod Kone