You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Adam B <ad...@mesosphere.io> on 2014/12/31 10:38:58 UTC
Review Request 29507: Added Configurable Slave Ping Timeouts
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/
-----------------------------------------------------------
Review request for mesos and Niklas Nielsen.
Bugs: MESOS-2150
https://issues.apache.org/jira/browse/MESOS-2150
Repository: mesos-git
Description
-------
Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
to mesos-master to replace the existing (still default)
SLAVE_PING_TIMEOUT (15secs) and MAX_SLAVE_PING_TIMEOUTS (5).
These can be extended if slaves are expected/allowed to be down for
longer than a minute or two.
Beware that this affects recovery from network timeouts as well as
actual slave node/process failover.
Diffs
-----
src/master/flags.hpp f5c8d2a8cc7530bc8746935af9ea90af747cc111
src/master/master.cpp d6651e299ddb73bfdc1b126c474075db6cda8acd
src/tests/fault_tolerance_tests.cpp 5763486acb6d687b50c02c01ea00e1cfbea48421
src/tests/mesos.cpp 3b98c69a604132be71a60fbbee4a47b51fe6956a
src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
src/tests/slave_recovery_tests.cpp cd4a398ef680b5694cb6069b8e2ca4e2c05911d1
src/tests/slave_tests.cpp c50cbc799d4793243f184f9fe628b69a020adc66
Diff: https://reviews.apache.org/r/29507/diff/
Testing
-------
Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
Updated unit tests to use shorter non-default values for ping timeouts.
Thanks,
Adam B
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review73085
-----------------------------------------------------------
Bad patch!
Reviews applied: [29507]
Failed command: ./support/apply-review.sh -n -r 29507
Error:
2015-02-19 04:34:42 URL:https://reviews.apache.org/r/29507/diff/raw/ [15458/15458] -> "29507.patch" [1]
error: patch failed: src/master/master.cpp:116
error: src/master/master.cpp: patch does not apply
error: patch failed: src/slave/constants.hpp:116
error: src/slave/constants.hpp: patch does not apply
error: patch failed: src/tests/partition_tests.cpp:67
error: src/tests/partition_tests.cpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On Feb. 19, 2015, 12:33 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2015, 12:33 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/constants.hpp c386eab
> src/master/constants.cpp 9ee17e9
> src/master/flags.hpp 6c18a1a
> src/master/master.cpp f4b6463
> src/slave/constants.hpp 761cfaf
> src/slave/constants.cpp 83d9fc1
> src/slave/slave.cpp a8b2621
> src/tests/fault_tolerance_tests.cpp f927d4a
> src/tests/partition_tests.cpp fea7801
> src/tests/slave_recovery_tests.cpp 7e2e63d
> src/tests/slave_tests.cpp e7e2af6
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
> On May 21, 2015, 9:41 a.m., Niklas Nielsen wrote:
> > Hey Adam, is this ready for review?
Yup. Ready for another round of review again.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review84726
-----------------------------------------------------------
On May 28, 2015, 4:13 p.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated May 28, 2015, 4:13 p.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
> Also fixed the log message in recoveredSlavesTimeout() to correctly
> reference flags.slave_reregister_timeout instead of the unrelated
> ping timeouts.
>
>
> Diffs
> -----
>
> docs/configuration.md 5a41477
> docs/upgrades.md 355307a
> src/master/constants.hpp 57cf8fb
> src/master/constants.cpp 8c7174a
> src/master/flags.hpp 84fa238
> src/master/flags.cpp 49d953a
> src/master/master.cpp 1526f59
> src/messages/messages.proto 39dac72
> src/slave/constants.hpp 206d439
> src/slave/constants.cpp d8d2f98
> src/slave/slave.hpp 0207eaf
> src/slave/slave.cpp b4d2029
> src/tests/partition_tests.cpp f7ee3ab
> src/tests/slave_recovery_tests.cpp c036e9c
> src/tests/slave_tests.cpp acae497
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review84726
-----------------------------------------------------------
Hey Adam, is this ready for review?
- Niklas Nielsen
On May 14, 2015, 3:01 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated May 14, 2015, 3:01 a.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> docs/configuration.md 54c4e31
> docs/upgrades.md 2a15694
> src/master/constants.hpp c386eab
> src/master/constants.cpp 9ee17e9
> src/master/flags.hpp 996cf38
> src/master/flags.cpp 5798989
> src/master/master.cpp eaea79d
> src/messages/messages.proto 19e2444
> src/slave/constants.hpp df02043
> src/slave/constants.cpp 07f699a
> src/slave/slave.hpp b62ed7b
> src/slave/slave.cpp 132f83e
> src/tests/partition_tests.cpp f7ee3ab
> src/tests/slave_recovery_tests.cpp c036e9c
> src/tests/slave_tests.cpp acae497
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Joerg Schad <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review83749
-----------------------------------------------------------
Ship it!
LGTM, minor readibility improvement suggestion.
docs/configuration.md
<https://reviews.apache.org/r/29507/#comment134815>
Slaves that incure more than
max_slave_ping_timeouts timeouts will be asked to shutdown.
- Joerg Schad
On May 14, 2015, 10:01 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated May 14, 2015, 10:01 a.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> docs/configuration.md 54c4e31
> docs/upgrades.md 2a15694
> src/master/constants.hpp c386eab
> src/master/constants.cpp 9ee17e9
> src/master/flags.hpp 996cf38
> src/master/flags.cpp 5798989
> src/master/master.cpp eaea79d
> src/messages/messages.proto 19e2444
> src/slave/constants.hpp df02043
> src/slave/constants.cpp 07f699a
> src/slave/slave.hpp b62ed7b
> src/slave/slave.cpp 132f83e
> src/tests/partition_tests.cpp f7ee3ab
> src/tests/slave_recovery_tests.cpp c036e9c
> src/tests/slave_tests.cpp acae497
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review89242
-----------------------------------------------------------
Ship it!
Thanks Adam, looks good, just one suggestion related to naming the field in the connection message.
docs/configuration.md (line 373)
<https://reviews.apache.org/r/29507/#comment141828>
How about "will be removed" to keep the terminology consistent with our other slave removal flag names, instead of "will be asked to shutdown"?
docs/configuration.md (lines 556 - 558)
<https://reviews.apache.org/r/29507/#comment141848>
Ditto here, let's say "removed" instead of "asked to shutdown".
docs/upgrades.md (line 13)
<https://reviews.apache.org/r/29507/#comment141850>
How about:
"Slaves should be upgraded to 0.23.x before changing this flag."
Just because the re-register comment here seems a bit confusing, people using this timeout might not know that the slaves use it to trigger re-registrations..? Up to you.
src/master/flags.cpp (lines 387 - 391)
<https://reviews.apache.org/r/29507/#comment141857>
"1s" and "15min" will not parse as Durations, but "1secs" and "15mins" will, so how about just stringifying directly, so we are guaranteed to show them a parseable string:
```
if (value < Seconds(1) || values > Minutes(15)) {
return Error("Expected --slave_ping_timeout to be within " +
stringify(Seconds(1)) + " and " +
stringify(Minutes(15)));
}
return None();
```
Also, it's too bad that we have to include the flag names here, I was a bit surprised that the load error coming from flags does not already include which flag's validation failed! I left a comment for benh here: https://reviews.apache.org/r/34943/
src/master/flags.cpp (line 401)
<https://reviews.apache.org/r/29507/#comment141858>
(1) s/const// since we don't bother taking in native types as const arguments in our existing style.
(2) s/uint32_t/size_t/ since we've generally been using 'size_t' for non-negative countable things like this.
src/master/master.cpp (line 129)
<https://reviews.apache.org/r/29507/#comment141859>
Ditto above: s/uint32_t/size_t/ and s/const//
Just to be consistent with the majority of our code.
src/master/master.cpp (line 269)
<https://reviews.apache.org/r/29507/#comment141860>
Here, just s/uint32_t/size_t
src/master/master.cpp (lines 3154 - 3157)
<https://reviews.apache.org/r/29507/#comment141862>
Hm.. you have a ping_timeout in the message, but that doesn't correspond to the ping timeout? It seems a bit misleading that it corresponds to the shutdown timeout.
Two suggestions:
(1) Rename 'ping_timeout_seconds' to 'total_ping_timeout_seconds' to help us realize that this isn't the singular ping timeout, along with a small note on the message related to this.
(2) Add max_ping_timeouts to the connection message, and set both fields directly from the flags.
(1) seems simpler to me for now.
src/master/master.cpp (line 3234)
<https://reviews.apache.org/r/29507/#comment141863>
Feel free to omit 'const' for locally scoped variables like this, we should probably be careful about proliferating 'const', there are a ton we can add to the existing source but it will probably get too verbose and inconsistent :(
src/messages/messages.proto (lines 296 - 298)
<https://reviews.apache.org/r/29507/#comment141865>
Per the above comment, how about:
```
message MasterSlaveConnection {
// If no pings are received within the total timeout,
// the master will remove the slave.
optional double total_ping_timeout_seconds = 1;
}
```
src/slave/slave.cpp (line 976)
<https://reviews.apache.org/r/29507/#comment141866>
Revert this change? Looks odd that TERMINATING has the same two lines but doesn't have the newline.
- Ben Mahler
On June 22, 2015, 10:03 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 10:03 a.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
> Also fixed the log message in recoveredSlavesTimeout() to correctly
> reference flags.slave_reregister_timeout instead of the unrelated
> ping timeouts.
>
>
> Diffs
> -----
>
> docs/configuration.md aaf65bf
> docs/upgrades.md 355307a
> src/master/constants.hpp 072d59c
> src/master/constants.cpp 997b792
> src/master/flags.hpp 55ed3a9
> src/master/flags.cpp 4377715
> src/master/master.cpp 0135c15
> src/messages/messages.proto 1c8d79e
> src/slave/constants.hpp 84927e5
> src/slave/constants.cpp d8d2f98
> src/slave/slave.hpp f1cf3b8
> src/slave/slave.cpp 40c0c33
> src/tests/partition_tests.cpp f7ee3ab
> src/tests/slave_recovery_tests.cpp c036e9c
> src/tests/slave_tests.cpp 5030198
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
> On June 24, 2015, 6:41 p.m., Ben Mahler wrote:
> > Actually, we should think about one more thing, how does this interact with the zookeeper session timeout?
The hardcoded individual ping timeout (15secs) was previously longer than the default zk session timeout (10secs), but the zk session timeout is already configurable with no bounds/validation, so users could already change it to be longer than an individual ping timeout, or even the total ping timeout. Is this a bad idea? What would this mean?
This is mostly relevant to the slave-side (as a leader detector), in that a slave may timeout and decide to check for a new leading master to reregister with, but the zk session may have recently gone bad but not yet timed out. Since a zk session could go bad/timeout at any point along the way, I don't think making the ping timeout configurable will introduce any new potential errors. Rather, it would just be advisable to keep the zk session timeouts reasonably small so that the zk session is likely to be healthy whenever a slave needs to detect a new leader. We could introduce validation that the zk_session_timeout is shorter than the (total) ping timeout, but I'm not sure that's even necessary.
Does this make sense? I'm no ZK expert, so I defer to those with more experience.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review89305
-----------------------------------------------------------
On June 26, 2015, 3:12 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated June 26, 2015, 3:12 a.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
> Also fixed the log message in recoveredSlavesTimeout() to correctly
> reference flags.slave_reregister_timeout instead of the unrelated
> ping timeouts.
>
>
> Diffs
> -----
>
> docs/configuration.md aaf65bf
> docs/upgrades.md 73e503c
> src/master/constants.hpp 072d59c
> src/master/constants.cpp 997b792
> src/master/flags.hpp 55ed3a9
> src/master/flags.cpp 4377715
> src/master/master.cpp 0782b54
> src/messages/messages.proto 1c8d79e
> src/slave/constants.hpp 84927e5
> src/slave/constants.cpp d8d2f98
> src/slave/slave.hpp f1cf3b8
> src/slave/slave.cpp b3e1ccc
> src/tests/partition_tests.cpp f7ee3ab
> src/tests/slave_recovery_tests.cpp c036e9c
> src/tests/slave_tests.cpp e9002e8
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
> On June 24, 2015, 6:41 p.m., Ben Mahler wrote:
> > Actually, we should think about one more thing, how does this interact with the zookeeper session timeout?
>
> Adam B wrote:
> The hardcoded individual ping timeout (15secs) was previously longer than the default zk session timeout (10secs), but the zk session timeout is already configurable with no bounds/validation, so users could already change it to be longer than an individual ping timeout, or even the total ping timeout. Is this a bad idea? What would this mean?
> This is mostly relevant to the slave-side (as a leader detector), in that a slave may timeout and decide to check for a new leading master to reregister with, but the zk session may have recently gone bad but not yet timed out. Since a zk session could go bad/timeout at any point along the way, I don't think making the ping timeout configurable will introduce any new potential errors. Rather, it would just be advisable to keep the zk session timeouts reasonably small so that the zk session is likely to be healthy whenever a slave needs to detect a new leader. We could introduce validation that the zk_session_timeout is shorter than the (total) ping timeout, but I'm not sure that's even necessary.
> Does this make sense? I'm no ZK expert, so I defer to those with more experience.
>
> Ben Mahler wrote:
> Right, the ZK timeout is negotiated with ZooKeeper, so it gets bounded based on the ZK server configuration:
> http://zookeeper.apache.org/doc/r3.3.1/zookeeperAdmin.html
>
> Validating that the session timeout is less than the total ping timeout might be nice, otherwise every session expiration slave-side will trigger useless re-registration attempts. Mind just adding a NOTE that reflects this in the flag help? The validation might be nice, but the ZK session timeout is only really determined after negotation anyway..
Noted in the flag help.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review89305
-----------------------------------------------------------
On June 26, 2015, 3:12 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated June 26, 2015, 3:12 a.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
> Also fixed the log message in recoveredSlavesTimeout() to correctly
> reference flags.slave_reregister_timeout instead of the unrelated
> ping timeouts.
>
>
> Diffs
> -----
>
> docs/configuration.md aaf65bf
> docs/upgrades.md 73e503c
> src/master/constants.hpp 072d59c
> src/master/constants.cpp 997b792
> src/master/flags.hpp 55ed3a9
> src/master/flags.cpp 4377715
> src/master/master.cpp 0782b54
> src/messages/messages.proto 1c8d79e
> src/slave/constants.hpp 84927e5
> src/slave/constants.cpp d8d2f98
> src/slave/slave.hpp f1cf3b8
> src/slave/slave.cpp b3e1ccc
> src/tests/partition_tests.cpp f7ee3ab
> src/tests/slave_recovery_tests.cpp c036e9c
> src/tests/slave_tests.cpp e9002e8
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Ben Mahler <be...@gmail.com>.
> On June 25, 2015, 1:41 a.m., Ben Mahler wrote:
> > Actually, we should think about one more thing, how does this interact with the zookeeper session timeout?
>
> Adam B wrote:
> The hardcoded individual ping timeout (15secs) was previously longer than the default zk session timeout (10secs), but the zk session timeout is already configurable with no bounds/validation, so users could already change it to be longer than an individual ping timeout, or even the total ping timeout. Is this a bad idea? What would this mean?
> This is mostly relevant to the slave-side (as a leader detector), in that a slave may timeout and decide to check for a new leading master to reregister with, but the zk session may have recently gone bad but not yet timed out. Since a zk session could go bad/timeout at any point along the way, I don't think making the ping timeout configurable will introduce any new potential errors. Rather, it would just be advisable to keep the zk session timeouts reasonably small so that the zk session is likely to be healthy whenever a slave needs to detect a new leader. We could introduce validation that the zk_session_timeout is shorter than the (total) ping timeout, but I'm not sure that's even necessary.
> Does this make sense? I'm no ZK expert, so I defer to those with more experience.
Right, the ZK timeout is negotiated with ZooKeeper, so it gets bounded based on the ZK server configuration:
http://zookeeper.apache.org/doc/r3.3.1/zookeeperAdmin.html
Validating that the session timeout is less than the total ping timeout might be nice, otherwise every session expiration slave-side will trigger useless re-registration attempts. Mind just adding a NOTE that reflects this in the flag help? The validation might be nice, but the ZK session timeout is only really determined after negotation anyway..
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review89305
-----------------------------------------------------------
On June 26, 2015, 10:12 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated June 26, 2015, 10:12 a.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
> Also fixed the log message in recoveredSlavesTimeout() to correctly
> reference flags.slave_reregister_timeout instead of the unrelated
> ping timeouts.
>
>
> Diffs
> -----
>
> docs/configuration.md aaf65bf
> docs/upgrades.md 73e503c
> src/master/constants.hpp 072d59c
> src/master/constants.cpp 997b792
> src/master/flags.hpp 55ed3a9
> src/master/flags.cpp 4377715
> src/master/master.cpp 0782b54
> src/messages/messages.proto 1c8d79e
> src/slave/constants.hpp 84927e5
> src/slave/constants.cpp d8d2f98
> src/slave/slave.hpp f1cf3b8
> src/slave/slave.cpp b3e1ccc
> src/tests/partition_tests.cpp f7ee3ab
> src/tests/slave_recovery_tests.cpp c036e9c
> src/tests/slave_tests.cpp e9002e8
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review89305
-----------------------------------------------------------
Actually, we should think about one more thing, how does this interact with the zookeeper session timeout?
- Ben Mahler
On June 22, 2015, 10:03 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 10:03 a.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
> Also fixed the log message in recoveredSlavesTimeout() to correctly
> reference flags.slave_reregister_timeout instead of the unrelated
> ping timeouts.
>
>
> Diffs
> -----
>
> docs/configuration.md aaf65bf
> docs/upgrades.md 355307a
> src/master/constants.hpp 072d59c
> src/master/constants.cpp 997b792
> src/master/flags.hpp 55ed3a9
> src/master/flags.cpp 4377715
> src/master/master.cpp 0135c15
> src/messages/messages.proto 1c8d79e
> src/slave/constants.hpp 84927e5
> src/slave/constants.cpp d8d2f98
> src/slave/slave.hpp f1cf3b8
> src/slave/slave.cpp 40c0c33
> src/tests/partition_tests.cpp f7ee3ab
> src/tests/slave_recovery_tests.cpp c036e9c
> src/tests/slave_tests.cpp 5030198
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review89482
-----------------------------------------------------------
Patch looks great!
Reviews applied: [29507]
All tests passed.
- Mesos ReviewBot
On June 26, 2015, 10:12 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated June 26, 2015, 10:12 a.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
> Also fixed the log message in recoveredSlavesTimeout() to correctly
> reference flags.slave_reregister_timeout instead of the unrelated
> ping timeouts.
>
>
> Diffs
> -----
>
> docs/configuration.md aaf65bf
> docs/upgrades.md 73e503c
> src/master/constants.hpp 072d59c
> src/master/constants.cpp 997b792
> src/master/flags.hpp 55ed3a9
> src/master/flags.cpp 4377715
> src/master/master.cpp 0782b54
> src/messages/messages.proto 1c8d79e
> src/slave/constants.hpp 84927e5
> src/slave/constants.cpp d8d2f98
> src/slave/slave.hpp f1cf3b8
> src/slave/slave.cpp b3e1ccc
> src/tests/partition_tests.cpp f7ee3ab
> src/tests/slave_recovery_tests.cpp c036e9c
> src/tests/slave_tests.cpp e9002e8
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/
-----------------------------------------------------------
(Updated June 26, 2015, 3:12 a.m.)
Review request for mesos, Ben Mahler and Niklas Nielsen.
Changes
-------
Updated in response to bmahler's comments.
Bugs: MESOS-2110
https://issues.apache.org/jira/browse/MESOS-2110
Repository: mesos
Description
-------
Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
These can be extended if slaves are expected/allowed to be down for
longer than a minute or two.
Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
Beware that this affects recovery from network timeouts as well as
actual slave node/process failover.
Also fixed the log message in recoveredSlavesTimeout() to correctly
reference flags.slave_reregister_timeout instead of the unrelated
ping timeouts.
Diffs (updated)
-----
docs/configuration.md aaf65bf
docs/upgrades.md 73e503c
src/master/constants.hpp 072d59c
src/master/constants.cpp 997b792
src/master/flags.hpp 55ed3a9
src/master/flags.cpp 4377715
src/master/master.cpp 0782b54
src/messages/messages.proto 1c8d79e
src/slave/constants.hpp 84927e5
src/slave/constants.cpp d8d2f98
src/slave/slave.hpp f1cf3b8
src/slave/slave.cpp b3e1ccc
src/tests/partition_tests.cpp f7ee3ab
src/tests/slave_recovery_tests.cpp c036e9c
src/tests/slave_tests.cpp e9002e8
Diff: https://reviews.apache.org/r/29507/diff/
Testing
-------
Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
Ran unit tests with shorter non-default values for ping timeouts.
`make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
Thanks,
Adam B
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review88761
-----------------------------------------------------------
Patch looks great!
Reviews applied: [29507]
All tests passed.
- Mesos ReviewBot
On June 22, 2015, 10:03 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 10:03 a.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
> Also fixed the log message in recoveredSlavesTimeout() to correctly
> reference flags.slave_reregister_timeout instead of the unrelated
> ping timeouts.
>
>
> Diffs
> -----
>
> docs/configuration.md aaf65bf
> docs/upgrades.md 355307a
> src/master/constants.hpp 072d59c
> src/master/constants.cpp 997b792
> src/master/flags.hpp 55ed3a9
> src/master/flags.cpp 4377715
> src/master/master.cpp 0135c15
> src/messages/messages.proto 1c8d79e
> src/slave/constants.hpp 84927e5
> src/slave/constants.cpp d8d2f98
> src/slave/slave.hpp f1cf3b8
> src/slave/slave.cpp 40c0c33
> src/tests/partition_tests.cpp f7ee3ab
> src/tests/slave_recovery_tests.cpp c036e9c
> src/tests/slave_tests.cpp 5030198
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review89243
-----------------------------------------------------------
Ship it!
Ship It!
- Niklas Nielsen
On June 22, 2015, 3:03 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 3:03 a.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
> Also fixed the log message in recoveredSlavesTimeout() to correctly
> reference flags.slave_reregister_timeout instead of the unrelated
> ping timeouts.
>
>
> Diffs
> -----
>
> docs/configuration.md aaf65bf
> docs/upgrades.md 355307a
> src/master/constants.hpp 072d59c
> src/master/constants.cpp 997b792
> src/master/flags.hpp 55ed3a9
> src/master/flags.cpp 4377715
> src/master/master.cpp 0135c15
> src/messages/messages.proto 1c8d79e
> src/slave/constants.hpp 84927e5
> src/slave/constants.cpp d8d2f98
> src/slave/slave.hpp f1cf3b8
> src/slave/slave.cpp 40c0c33
> src/tests/partition_tests.cpp f7ee3ab
> src/tests/slave_recovery_tests.cpp c036e9c
> src/tests/slave_tests.cpp 5030198
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/
-----------------------------------------------------------
(Updated June 22, 2015, 3:03 a.m.)
Review request for mesos, Ben Mahler and Niklas Nielsen.
Changes
-------
Updated with Niklas and BenM's suggestions. Excluding new unit tests; will post subsequent review with them.
Bugs: MESOS-2110
https://issues.apache.org/jira/browse/MESOS-2110
Repository: mesos
Description
-------
Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
These can be extended if slaves are expected/allowed to be down for
longer than a minute or two.
Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
Beware that this affects recovery from network timeouts as well as
actual slave node/process failover.
Also fixed the log message in recoveredSlavesTimeout() to correctly
reference flags.slave_reregister_timeout instead of the unrelated
ping timeouts.
Diffs (updated)
-----
docs/configuration.md aaf65bf
docs/upgrades.md 355307a
src/master/constants.hpp 072d59c
src/master/constants.cpp 997b792
src/master/flags.hpp 55ed3a9
src/master/flags.cpp 4377715
src/master/master.cpp 0135c15
src/messages/messages.proto 1c8d79e
src/slave/constants.hpp 84927e5
src/slave/constants.cpp d8d2f98
src/slave/slave.hpp f1cf3b8
src/slave/slave.cpp 40c0c33
src/tests/partition_tests.cpp f7ee3ab
src/tests/slave_recovery_tests.cpp c036e9c
src/tests/slave_tests.cpp 5030198
Diff: https://reviews.apache.org/r/29507/diff/
Testing
-------
Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
Ran unit tests with shorter non-default values for ping timeouts.
`make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
Thanks,
Adam B
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
> On June 3, 2015, 2:24 p.m., Ben Mahler wrote:
> > Overall it's looking pretty good! I'd suggest splitting out the tests given the comments on the source changes are fairly minor.
> >
> > Echoing Nik, it would be nice to have some sanity bound checks for this flag, to prevent operators from burning themselves.
Will do. This next revision excludes the new tests, and I will address the test comments in a subsequent patch/review.
Added flag validation using the cool new lambda style from https://reviews.apache.org/r/34943
> On June 3, 2015, 2:24 p.m., Ben Mahler wrote:
> > src/slave/slave.hpp, lines 100-109
> > <https://reviews.apache.org/r/29507/diff/7/?file=973886#file973886line100>
> >
> > Ditto Nik's comment for camelCase, but also, could you update these to use `Duration` rather than directly using a `double`?
Removed `double ping_timeout_seconds` in favor of the nested submessage `connection.ping_timeout_seconds()` as you suggested later.
> On June 3, 2015, 2:24 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, lines 885-888
> > <https://reviews.apache.org/r/29507/diff/7/?file=973887#file973887line885>
> >
> > Could you move this up and do it once before the switch?
> >
> > I see now why you probably wanted the explicit default, it's too bad we don't have support to bind in `Option`s for optional fields in install handlers. How about we rely on the implicit default: https://developers.google.com/protocol-buffers/docs/proto#optional
> >
> > Alternatively, you could have a nested message that contains the fields (e.g. 'Connection') which allows you to explicitly check for '`has_ping_timeout_seconds`').
Excellent points. I moved the check up before the switch, and I went with your nested submessage approach instead of an optional default. I think that reads much better.
And yes, I wanted the explicit default so I could check for a registration from a pre-0.23 master, to maintain the old behavior. I just realized that even this was not enough, in case this slave had registered with a newer master, then reregistered with an older master, so I have explicitly added an else case to reset it to the old DEFAULT_MASTER_PING_TIMEOUT().
> On June 3, 2015, 2:24 p.m., Ben Mahler wrote:
> > src/messages/messages.proto, lines 277-288
> > <https://reviews.apache.org/r/29507/diff/7/?file=973883#file973883line277>
> >
> > The defaults here seem to suggest that we will rely on them (i.e. we don't check if the field is set, rather we always grab the value). However, 0 is going to be a problematic value! I'd suggest removing the defaults, and rather, adding a comment here for posterity that indicates that these were added in 0.23.0.
Addressed with your suggestion of a nested submessage instead.
> On June 3, 2015, 2:24 p.m., Ben Mahler wrote:
> > src/slave/slave.cpp, line 923
> > <https://reviews.apache.org/r/29507/diff/7/?file=973887#file973887line923>
> >
> > "once re-registered"?
Fixed.
> On June 3, 2015, 2:24 p.m., Ben Mahler wrote:
> > src/tests/partition_tests.cpp, lines 600-602
> > <https://reviews.apache.org/r/29507/diff/7/?file=973888#file973888line600>
> >
> > I'm a bit confused by this test, why are you expecting the first ping? Why are you dropping the pongs?
> >
> > I would expect the pings to be dropped entirely here so that advancing the clock after a slave is registered leads to a re-registration.
> >
> > Also, did you know about SlaveTest.PingTimeoutNoPings and SlaveTest.PingTimeoutSomePings tests? Is there something being tested here that is not captured in these?
This test was intended to simulate a one-way partition where the slave can no longer reach the master, after the slave initially successfully registered. Hence the PONGs are dropped, but not the PINGs.
> On June 3, 2015, 2:24 p.m., Ben Mahler wrote:
> > src/tests/partition_tests.cpp, lines 647-650
> > <https://reviews.apache.org/r/29507/diff/7/?file=973888#file973888line647>
> >
> > This sounds like testing two particular behaviors in a single test:
> >
> > (1) A registered slave that never receives any Ping messages will attempt to reregister after the master's ping timeout.
> >
> > (2) Master will consider it shutdown after the same timeout.
> >
> > I believe we already have tests for both of these. For (1) we could just add the shutdown message expectation to `PartitionTest.PartitionedSlave`. For (2), we have SlaveTest.PingTimeoutNoPings, SlaveTest.PingTimeoutNoPings.
Yes, I see now that SlaveTest.PingTimeoutNoPings already satisfies (1), and PartitionTest.PartitionedSlave could be modified to check for the ShutdownMessage() to satisfy (2).
I originally started trying to write unit tests for registered/unregistered slaves that encountered each type of a one-way partition, with both shorter and longer-than-default ping timeouts, largely for my own understanding of the process. Testing longer timeouts proved unnecessary, and testing unregistered slaves didn't actually test my feature. I posted these two since they worked and explicitly tested the one-way partitions I was trying to test. But this one can almost certainly be covered by the tests you mentioned. I will update in a follow-up patch.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review86461
-----------------------------------------------------------
On May 28, 2015, 4:13 p.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated May 28, 2015, 4:13 p.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
> Also fixed the log message in recoveredSlavesTimeout() to correctly
> reference flags.slave_reregister_timeout instead of the unrelated
> ping timeouts.
>
>
> Diffs
> -----
>
> docs/configuration.md 5a41477
> docs/upgrades.md 355307a
> src/master/constants.hpp 57cf8fb
> src/master/constants.cpp 8c7174a
> src/master/flags.hpp 84fa238
> src/master/flags.cpp 49d953a
> src/master/master.cpp 1526f59
> src/messages/messages.proto 39dac72
> src/slave/constants.hpp 206d439
> src/slave/constants.cpp d8d2f98
> src/slave/slave.hpp 0207eaf
> src/slave/slave.cpp b4d2029
> src/tests/partition_tests.cpp f7ee3ab
> src/tests/slave_recovery_tests.cpp c036e9c
> src/tests/slave_tests.cpp acae497
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
> On June 3, 2015, 2:24 p.m., Ben Mahler wrote:
> > src/tests/partition_tests.cpp, lines 647-650
> > <https://reviews.apache.org/r/29507/diff/7/?file=973888#file973888line647>
> >
> > This sounds like testing two particular behaviors in a single test:
> >
> > (1) A registered slave that never receives any Ping messages will attempt to reregister after the master's ping timeout.
> >
> > (2) Master will consider it shutdown after the same timeout.
> >
> > I believe we already have tests for both of these. For (1) we could just add the shutdown message expectation to `PartitionTest.PartitionedSlave`. For (2), we have SlaveTest.PingTimeoutNoPings, SlaveTest.PingTimeoutNoPings.
>
> Adam B wrote:
> Yes, I see now that SlaveTest.PingTimeoutNoPings already satisfies (1), and PartitionTest.PartitionedSlave could be modified to check for the ShutdownMessage() to satisfy (2).
> I originally started trying to write unit tests for registered/unregistered slaves that encountered each type of a one-way partition, with both shorter and longer-than-default ping timeouts, largely for my own understanding of the process. Testing longer timeouts proved unnecessary, and testing unregistered slaves didn't actually test my feature. I posted these two since they worked and explicitly tested the one-way partitions I was trying to test. But this one can almost certainly be covered by the tests you mentioned. I will update in a follow-up patch.
It turns out that the shutdown message expectation is already covered in SlaveRecoveryTest.PartitionedSlave, which is similar to, but in many ways a superset of PartitionTest.PartitionedSlave. I have removed my two new tests completely and relied on SlaveRecoveryTest.PartitionedSlave and SlaveTest.PingTimeoutNoPings, with the latter updated to use custom ping timeout values.
For the updated test, see new RR: https://reviews.apache.org/r/35958
> On June 3, 2015, 2:24 p.m., Ben Mahler wrote:
> > src/tests/partition_tests.cpp, lines 600-602
> > <https://reviews.apache.org/r/29507/diff/7/?file=973888#file973888line600>
> >
> > I'm a bit confused by this test, why are you expecting the first ping? Why are you dropping the pongs?
> >
> > I would expect the pings to be dropped entirely here so that advancing the clock after a slave is registered leads to a re-registration.
> >
> > Also, did you know about SlaveTest.PingTimeoutNoPings and SlaveTest.PingTimeoutSomePings tests? Is there something being tested here that is not captured in these?
>
> Adam B wrote:
> This test was intended to simulate a one-way partition where the slave can no longer reach the master, after the slave initially successfully registered. Hence the PONGs are dropped, but not the PINGs.
Even though we're dropping PONGs instead of PINGs here, it's still only testing that the Slave reregisters after the timeout. If anything, it should be testing that a master that never receives PONGs will shutdown the slave. This behavior is already tested by SlaveRecoveryTest.PartitionedSlave. No new test needed.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review86461
-----------------------------------------------------------
On June 26, 2015, 3:12 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated June 26, 2015, 3:12 a.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
> Also fixed the log message in recoveredSlavesTimeout() to correctly
> reference flags.slave_reregister_timeout instead of the unrelated
> ping timeouts.
>
>
> Diffs
> -----
>
> docs/configuration.md aaf65bf
> docs/upgrades.md 73e503c
> src/master/constants.hpp 072d59c
> src/master/constants.cpp 997b792
> src/master/flags.hpp 55ed3a9
> src/master/flags.cpp 4377715
> src/master/master.cpp 0782b54
> src/messages/messages.proto 1c8d79e
> src/slave/constants.hpp 84927e5
> src/slave/constants.cpp d8d2f98
> src/slave/slave.hpp f1cf3b8
> src/slave/slave.cpp b3e1ccc
> src/tests/partition_tests.cpp f7ee3ab
> src/tests/slave_recovery_tests.cpp c036e9c
> src/tests/slave_tests.cpp e9002e8
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review86461
-----------------------------------------------------------
Overall it's looking pretty good! I'd suggest splitting out the tests given the comments on the source changes are fairly minor.
Echoing Nik, it would be nice to have some sanity bound checks for this flag, to prevent operators from burning themselves.
src/master/master.cpp
<https://reviews.apache.org/r/29507/#comment138489>
I'm guessing the snake_case came from the flag names, but this should use camelCase.
src/master/master.cpp
<https://reviews.apache.org/r/29507/#comment138524>
Duration exposes .secs() for this purpose:
s/Seconds(pingTimeout).value()/pingTimeout.secs()/
Can you update it here and in the other cases below?
src/messages/messages.proto
<https://reviews.apache.org/r/29507/#comment138491>
The defaults here seem to suggest that we will rely on them (i.e. we don't check if the field is set, rather we always grab the value). However, 0 is going to be a problematic value! I'd suggest removing the defaults, and rather, adding a comment here for posterity that indicates that these were added in 0.23.0.
src/slave/slave.hpp
<https://reviews.apache.org/r/29507/#comment138493>
Ditto Nik's comment for camelCase, but also, could you update these to use `Duration` rather than directly using a `double`?
src/slave/slave.cpp
<https://reviews.apache.org/r/29507/#comment138520>
Could you move this up and do it once before the switch?
I see now why you probably wanted the explicit default, it's too bad we don't have support to bind in `Option`s for optional fields in install handlers. How about we rely on the implicit default: https://developers.google.com/protocol-buffers/docs/proto#optional
Alternatively, you could have a nested message that contains the fields (e.g. 'Connection') which allows you to explicitly check for '`has_ping_timeout_seconds`').
src/slave/slave.cpp
<https://reviews.apache.org/r/29507/#comment138522>
"once re-registered"?
src/tests/partition_tests.cpp
<https://reviews.apache.org/r/29507/#comment138527>
I'm a bit confused by this test, why are you expecting the first ping? Why are you dropping the pongs?
I would expect the pings to be dropped entirely here so that advancing the clock after a slave is registered leads to a re-registration.
Also, did you know about SlaveTest.PingTimeoutNoPings and SlaveTest.PingTimeoutSomePings tests? Is there something being tested here that is not captured in these?
src/tests/partition_tests.cpp
<https://reviews.apache.org/r/29507/#comment138525>
Can you do a sweep and replace all of these Seconds(duration).value() cases with duration.secs()?
src/tests/partition_tests.cpp
<https://reviews.apache.org/r/29507/#comment138529>
This sounds like testing two particular behaviors in a single test:
(1) A registered slave that never receives any Ping messages will attempt to reregister after the master's ping timeout.
(2) Master will consider it shutdown after the same timeout.
I believe we already have tests for both of these. For (1) we could just add the shutdown message expectation to `PartitionTest.PartitionedSlave`. For (2), we have SlaveTest.PingTimeoutNoPings, SlaveTest.PingTimeoutNoPings.
- Ben Mahler
On May 28, 2015, 11:13 p.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated May 28, 2015, 11:13 p.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
> Also fixed the log message in recoveredSlavesTimeout() to correctly
> reference flags.slave_reregister_timeout instead of the unrelated
> ping timeouts.
>
>
> Diffs
> -----
>
> docs/configuration.md 5a41477
> docs/upgrades.md 355307a
> src/master/constants.hpp 57cf8fb
> src/master/constants.cpp 8c7174a
> src/master/flags.hpp 84fa238
> src/master/flags.cpp 49d953a
> src/master/master.cpp 1526f59
> src/messages/messages.proto 39dac72
> src/slave/constants.hpp 206d439
> src/slave/constants.cpp d8d2f98
> src/slave/slave.hpp 0207eaf
> src/slave/slave.cpp b4d2029
> src/tests/partition_tests.cpp f7ee3ab
> src/tests/slave_recovery_tests.cpp c036e9c
> src/tests/slave_tests.cpp acae497
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
> On June 1, 2015, 1:15 p.m., Niklas Nielsen wrote:
> > LGTM - Would it make sense to have sane min/max values for the timeouts/counts?
> >
> > I wonder it would make sense to have a test to exercise an upgrade path (the timeout being different in the slaves, than in the master). Maybe I missed that in your first test.
Added flag validation using the cool new lambda style from https://reviews.apache.org/r/34943
It's not so easy to test the upgrade path in a unit test, since as soon as a slave from this source base gets its SlaveRegistered ack from the master, it then has the same timeout as the master (as designed). And if the slave never receives its registered ack, it will retry registration after a /random/ backoff initially capped at 1min (--registration_backoff_factor). If the random backoff_factor ends up being longer than the slave's ping timeout, when the ping timeout expires, the slave will anyway re-detect the master and retry registration. As I revisit the tests (in a follow-up patch), I will brainstorm ways to test the upgrade path.
> On June 1, 2015, 1:15 p.m., Niklas Nielsen wrote:
> > src/messages/messages.proto, line 279
> > <https://reviews.apache.org/r/29507/diff/7/?file=973883#file973883line279>
> >
> > Why a zero default?
Originally added as a way for the recipient of this message to know that the sender didn't set it, presumably because the sender is pre-0.23. Since we don't have a way for install handlers to translate optional fields into Options instead of just pulling in the default value, I couldn't think of a cleaner option. BenM, however, suggested pushing the ping timeout into a nested submessage, so I could just check has_ping_timeout_seconds() instead. Went with that instead.
> On June 1, 2015, 1:15 p.m., Niklas Nielsen wrote:
> > src/slave/slave.cpp, line 864
> > <https://reviews.apache.org/r/29507/diff/7/?file=973887#file973887line864>
> >
> > If we removed the 0 default, we'd do:
> >
> > if (ping_timeout_seconds.isSome()) {
> > // ...
> > }
> >
> > Which seems a bit more logical to me :)
Unfortunately, we don't currently translate optional protobuf fields into Options in install handlers, so I went for an explicit default. BenM suggested using a nested message so I can check `if (connection.has_ping_timeout_seconds())` instead. Much more readable.
> On June 1, 2015, 1:15 p.m., Niklas Nielsen wrote:
> > docs/configuration.md, line 339
> > <https://reviews.apache.org/r/29507/diff/7/?file=973876#file973876line339>
> >
> > As this will be markdown, how about formatting max_slave_ping_timeouts in `` or as "--max_slave_ping_timeouts" ?
Fixed.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review86039
-----------------------------------------------------------
On May 28, 2015, 4:13 p.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated May 28, 2015, 4:13 p.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
> Also fixed the log message in recoveredSlavesTimeout() to correctly
> reference flags.slave_reregister_timeout instead of the unrelated
> ping timeouts.
>
>
> Diffs
> -----
>
> docs/configuration.md 5a41477
> docs/upgrades.md 355307a
> src/master/constants.hpp 57cf8fb
> src/master/constants.cpp 8c7174a
> src/master/flags.hpp 84fa238
> src/master/flags.cpp 49d953a
> src/master/master.cpp 1526f59
> src/messages/messages.proto 39dac72
> src/slave/constants.hpp 206d439
> src/slave/constants.cpp d8d2f98
> src/slave/slave.hpp 0207eaf
> src/slave/slave.cpp b4d2029
> src/tests/partition_tests.cpp f7ee3ab
> src/tests/slave_recovery_tests.cpp c036e9c
> src/tests/slave_tests.cpp acae497
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review86039
-----------------------------------------------------------
LGTM - Would it make sense to have sane min/max values for the timeouts/counts?
I wonder it would make sense to have a test to exercise an upgrade path (the timeout being different in the slaves, than in the master). Maybe I missed that in your first test.
docs/configuration.md
<https://reviews.apache.org/r/29507/#comment137889>
As this will be markdown, how about formatting max_slave_ping_timeouts in `` or as "--max_slave_ping_timeouts" ?
docs/upgrades.md
<https://reviews.apache.org/r/29507/#comment137891>
You shouldn't need this line - look at line 9 :)
src/master/master.cpp
<https://reviews.apache.org/r/29507/#comment137899>
You should be able to const this, right?
Here and below
src/messages/messages.proto
<https://reviews.apache.org/r/29507/#comment137902>
Why a zero default?
src/slave/slave.hpp
<https://reviews.apache.org/r/29507/#comment137903>
We should Camel case here, right?
src/slave/slave.hpp
<https://reviews.apache.org/r/29507/#comment137904>
Same here
src/slave/slave.cpp
<https://reviews.apache.org/r/29507/#comment137905>
Camel case?
src/slave/slave.cpp
<https://reviews.apache.org/r/29507/#comment137906>
If we removed the 0 default, we'd do:
if (ping_timeout_seconds.isSome()) {
// ...
}
Which seems a bit more logical to me :)
src/tests/partition_tests.cpp
<https://reviews.apache.org/r/29507/#comment137936>
What do you think about commenting on why you chose these dividing factors (Why 3, Why 2? What will these do compared to the original timeouts?)
src/tests/partition_tests.cpp
<https://reviews.apache.org/r/29507/#comment137931>
const?
src/tests/partition_tests.cpp
<https://reviews.apache.org/r/29507/#comment137938>
newline?
src/tests/partition_tests.cpp
<https://reviews.apache.org/r/29507/#comment137937>
Newline?
- Niklas Nielsen
On May 28, 2015, 4:13 p.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated May 28, 2015, 4:13 p.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
> Also fixed the log message in recoveredSlavesTimeout() to correctly
> reference flags.slave_reregister_timeout instead of the unrelated
> ping timeouts.
>
>
> Diffs
> -----
>
> docs/configuration.md 5a41477
> docs/upgrades.md 355307a
> src/master/constants.hpp 57cf8fb
> src/master/constants.cpp 8c7174a
> src/master/flags.hpp 84fa238
> src/master/flags.cpp 49d953a
> src/master/master.cpp 1526f59
> src/messages/messages.proto 39dac72
> src/slave/constants.hpp 206d439
> src/slave/constants.cpp d8d2f98
> src/slave/slave.hpp 0207eaf
> src/slave/slave.cpp b4d2029
> src/tests/partition_tests.cpp f7ee3ab
> src/tests/slave_recovery_tests.cpp c036e9c
> src/tests/slave_tests.cpp acae497
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review85652
-----------------------------------------------------------
Patch looks great!
Reviews applied: [29507]
All tests passed.
- Mesos ReviewBot
On May 28, 2015, 11:13 p.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated May 28, 2015, 11:13 p.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
> Also fixed the log message in recoveredSlavesTimeout() to correctly
> reference flags.slave_reregister_timeout instead of the unrelated
> ping timeouts.
>
>
> Diffs
> -----
>
> docs/configuration.md 5a41477
> docs/upgrades.md 355307a
> src/master/constants.hpp 57cf8fb
> src/master/constants.cpp 8c7174a
> src/master/flags.hpp 84fa238
> src/master/flags.cpp 49d953a
> src/master/master.cpp 1526f59
> src/messages/messages.proto 39dac72
> src/slave/constants.hpp 206d439
> src/slave/constants.cpp d8d2f98
> src/slave/slave.hpp 0207eaf
> src/slave/slave.cpp b4d2029
> src/tests/partition_tests.cpp f7ee3ab
> src/tests/slave_recovery_tests.cpp c036e9c
> src/tests/slave_tests.cpp acae497
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review87574
-----------------------------------------------------------
Ping @Adam :) Let's get his in soon
- Niklas Nielsen
On May 28, 2015, 4:13 p.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated May 28, 2015, 4:13 p.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
> Also fixed the log message in recoveredSlavesTimeout() to correctly
> reference flags.slave_reregister_timeout instead of the unrelated
> ping timeouts.
>
>
> Diffs
> -----
>
> docs/configuration.md 5a41477
> docs/upgrades.md 355307a
> src/master/constants.hpp 57cf8fb
> src/master/constants.cpp 8c7174a
> src/master/flags.hpp 84fa238
> src/master/flags.cpp 49d953a
> src/master/master.cpp 1526f59
> src/messages/messages.proto 39dac72
> src/slave/constants.hpp 206d439
> src/slave/constants.cpp d8d2f98
> src/slave/slave.hpp 0207eaf
> src/slave/slave.cpp b4d2029
> src/tests/partition_tests.cpp f7ee3ab
> src/tests/slave_recovery_tests.cpp c036e9c
> src/tests/slave_tests.cpp acae497
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/
-----------------------------------------------------------
(Updated May 28, 2015, 4:13 p.m.)
Review request for mesos, Ben Mahler and Niklas Nielsen.
Changes
-------
Updated diff with minor feedback, updated description to mention slave_reregister_timeout fix.
Bugs: MESOS-2110
https://issues.apache.org/jira/browse/MESOS-2110
Repository: mesos
Description (updated)
-------
Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
These can be extended if slaves are expected/allowed to be down for
longer than a minute or two.
Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
Beware that this affects recovery from network timeouts as well as
actual slave node/process failover.
Also fixed the log message in recoveredSlavesTimeout() to correctly
reference flags.slave_reregister_timeout instead of the unrelated
ping timeouts.
Diffs (updated)
-----
docs/configuration.md 5a41477
docs/upgrades.md 355307a
src/master/constants.hpp 57cf8fb
src/master/constants.cpp 8c7174a
src/master/flags.hpp 84fa238
src/master/flags.cpp 49d953a
src/master/master.cpp 1526f59
src/messages/messages.proto 39dac72
src/slave/constants.hpp 206d439
src/slave/constants.cpp d8d2f98
src/slave/slave.hpp 0207eaf
src/slave/slave.cpp b4d2029
src/tests/partition_tests.cpp f7ee3ab
src/tests/slave_recovery_tests.cpp c036e9c
src/tests/slave_tests.cpp acae497
Diff: https://reviews.apache.org/r/29507/diff/
Testing
-------
Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
Ran unit tests with shorter non-default values for ping timeouts.
`make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
Thanks,
Adam B
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
> On May 14, 2015, 6:07 a.m., Alexander Rukletsov wrote:
> > src/master/master.cpp, line 1315
> > <https://reviews.apache.org/r/29507/diff/6/?file=959052#file959052line1315>
> >
> > This looks like a drive-by bug fix, should it be included in this diff? At least, let's mention it in the description.
Had to modify it when I was changing the constants, and upon closer inspection it was logging the wrong timeout anyway.
Mentioned it in the description now.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review83758
-----------------------------------------------------------
On May 14, 2015, 3:01 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated May 14, 2015, 3:01 a.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> docs/configuration.md 54c4e31
> docs/upgrades.md 2a15694
> src/master/constants.hpp c386eab
> src/master/constants.cpp 9ee17e9
> src/master/flags.hpp 996cf38
> src/master/flags.cpp 5798989
> src/master/master.cpp eaea79d
> src/messages/messages.proto 19e2444
> src/slave/constants.hpp df02043
> src/slave/constants.cpp 07f699a
> src/slave/slave.hpp b62ed7b
> src/slave/slave.cpp 132f83e
> src/tests/partition_tests.cpp f7ee3ab
> src/tests/slave_recovery_tests.cpp c036e9c
> src/tests/slave_tests.cpp acae497
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review83758
-----------------------------------------------------------
src/master/master.cpp
<https://reviews.apache.org/r/29507/#comment134820>
This looks like a drive-by bug fix, should it be included in this diff? At least, let's mention it in the description.
- Alexander Rukletsov
On May 14, 2015, 10:01 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated May 14, 2015, 10:01 a.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> docs/configuration.md 54c4e31
> docs/upgrades.md 2a15694
> src/master/constants.hpp c386eab
> src/master/constants.cpp 9ee17e9
> src/master/flags.hpp 996cf38
> src/master/flags.cpp 5798989
> src/master/master.cpp eaea79d
> src/messages/messages.proto 19e2444
> src/slave/constants.hpp df02043
> src/slave/constants.cpp 07f699a
> src/slave/slave.hpp b62ed7b
> src/slave/slave.cpp 132f83e
> src/tests/partition_tests.cpp f7ee3ab
> src/tests/slave_recovery_tests.cpp c036e9c
> src/tests/slave_tests.cpp acae497
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review83747
-----------------------------------------------------------
Patch looks great!
Reviews applied: [29507]
All tests passed.
- Mesos ReviewBot
On May 14, 2015, 10:01 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated May 14, 2015, 10:01 a.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> docs/configuration.md 54c4e31
> docs/upgrades.md 2a15694
> src/master/constants.hpp c386eab
> src/master/constants.cpp 9ee17e9
> src/master/flags.hpp 996cf38
> src/master/flags.cpp 5798989
> src/master/master.cpp eaea79d
> src/messages/messages.proto 19e2444
> src/slave/constants.hpp df02043
> src/slave/constants.cpp 07f699a
> src/slave/slave.hpp b62ed7b
> src/slave/slave.cpp 132f83e
> src/tests/partition_tests.cpp f7ee3ab
> src/tests/slave_recovery_tests.cpp c036e9c
> src/tests/slave_tests.cpp acae497
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/
-----------------------------------------------------------
(Updated May 14, 2015, 3:01 a.m.)
Review request for mesos, Ben Mahler and Niklas Nielsen.
Changes
-------
Rebased
Bugs: MESOS-2110
https://issues.apache.org/jira/browse/MESOS-2110
Repository: mesos
Description
-------
Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
These can be extended if slaves are expected/allowed to be down for
longer than a minute or two.
Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
Beware that this affects recovery from network timeouts as well as
actual slave node/process failover.
Diffs (updated)
-----
docs/configuration.md 54c4e31
docs/upgrades.md 2a15694
src/master/constants.hpp c386eab
src/master/constants.cpp 9ee17e9
src/master/flags.hpp 996cf38
src/master/flags.cpp 5798989
src/master/master.cpp eaea79d
src/messages/messages.proto 19e2444
src/slave/constants.hpp df02043
src/slave/constants.cpp 07f699a
src/slave/slave.hpp b62ed7b
src/slave/slave.cpp 132f83e
src/tests/partition_tests.cpp f7ee3ab
src/tests/slave_recovery_tests.cpp c036e9c
src/tests/slave_tests.cpp acae497
Diff: https://reviews.apache.org/r/29507/diff/
Testing
-------
Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
Ran unit tests with shorter non-default values for ping timeouts.
`make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
Thanks,
Adam B
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review83738
-----------------------------------------------------------
Bad patch!
Reviews applied: [29507]
Failed command: ./support/apply-review.sh -n -r 29507
Error:
2015-05-14 09:15:26 URL:https://reviews.apache.org/r/29507/diff/raw/ [33154/33154] -> "29507.patch" [1]
error: patch failed: src/slave/constants.hpp:102
error: src/slave/constants.hpp: patch does not apply
error: patch failed: src/slave/constants.cpp:52
error: src/slave/constants.cpp: patch does not apply
error: patch failed: src/tests/fault_tolerance_tests.cpp:144
error: src/tests/fault_tolerance_tests.cpp: patch does not apply
Failed to apply patch
- Mesos ReviewBot
On May 14, 2015, 8:54 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated May 14, 2015, 8:54 a.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> docs/configuration.md 54c4e31
> docs/upgrades.md 2a15694
> src/master/constants.hpp c386eab
> src/master/constants.cpp 9ee17e9
> src/master/flags.hpp 996cf38
> src/master/flags.cpp 5798989
> src/master/master.cpp ec32cd6
> src/messages/messages.proto 98d859f
> src/slave/constants.hpp fd1c1ab
> src/slave/constants.cpp 2a99b11
> src/slave/slave.hpp adb52b5
> src/slave/slave.cpp 1b17441
> src/tests/fault_tolerance_tests.cpp 3a27d82
> src/tests/partition_tests.cpp 1018e47
> src/tests/slave_recovery_tests.cpp c036e9c
> src/tests/slave_tests.cpp 04e79ec
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/
-----------------------------------------------------------
(Updated May 14, 2015, 1:54 a.m.)
Review request for mesos, Ben Mahler and Niklas Nielsen.
Changes
-------
Rebased and added configuration/upgrade documentation.
Bugs: MESOS-2110
https://issues.apache.org/jira/browse/MESOS-2110
Repository: mesos
Description
-------
Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
These can be extended if slaves are expected/allowed to be down for
longer than a minute or two.
Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
Beware that this affects recovery from network timeouts as well as
actual slave node/process failover.
Diffs (updated)
-----
docs/configuration.md 54c4e31
docs/upgrades.md 2a15694
src/master/constants.hpp c386eab
src/master/constants.cpp 9ee17e9
src/master/flags.hpp 996cf38
src/master/flags.cpp 5798989
src/master/master.cpp ec32cd6
src/messages/messages.proto 98d859f
src/slave/constants.hpp fd1c1ab
src/slave/constants.cpp 2a99b11
src/slave/slave.hpp adb52b5
src/slave/slave.cpp 1b17441
src/tests/fault_tolerance_tests.cpp 3a27d82
src/tests/partition_tests.cpp 1018e47
src/tests/slave_recovery_tests.cpp c036e9c
src/tests/slave_tests.cpp 04e79ec
Diff: https://reviews.apache.org/r/29507/diff/
Testing
-------
Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
Ran unit tests with shorter non-default values for ping timeouts.
`make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
Thanks,
Adam B
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review73111
-----------------------------------------------------------
Patch looks great!
Reviews applied: [29507]
All tests passed.
- Mesos ReviewBot
On Feb. 19, 2015, 8:10 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2015, 8:10 a.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/constants.hpp ad3fe81
> src/master/constants.cpp d3d0f71
> src/master/flags.hpp 51a6059
> src/master/master.cpp f10a3cf
> src/messages/messages.proto 58484ae
> src/slave/constants.hpp 12d6e92
> src/slave/constants.cpp 7868bef
> src/slave/slave.hpp 91dae10
> src/slave/slave.cpp aec9525
> src/tests/fault_tolerance_tests.cpp efa5c57
> src/tests/partition_tests.cpp eb16a58
> src/tests/slave_recovery_tests.cpp 8210c52
> src/tests/slave_tests.cpp 153d9d6
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
> On March 6, 2015, 5:19 a.m., Joerg Schad wrote:
> > src/master/flags.hpp, line 383
> > <https://reviews.apache.org/r/29507/diff/4/?file=868836#file868836line383>
> >
> > Shouldn't this also be added to the documentation (i.e. http://mesos.apache.org/documentation/latest/configuration/)?
Excellent point. Will update that in the next patch revision.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review75484
-----------------------------------------------------------
On Feb. 19, 2015, 12:10 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2015, 12:10 a.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/constants.hpp ad3fe81
> src/master/constants.cpp d3d0f71
> src/master/flags.hpp 51a6059
> src/master/master.cpp f10a3cf
> src/messages/messages.proto 58484ae
> src/slave/constants.hpp 12d6e92
> src/slave/constants.cpp 7868bef
> src/slave/slave.hpp 91dae10
> src/slave/slave.cpp aec9525
> src/tests/fault_tolerance_tests.cpp efa5c57
> src/tests/partition_tests.cpp eb16a58
> src/tests/slave_recovery_tests.cpp 8210c52
> src/tests/slave_tests.cpp 153d9d6
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Joerg Schad <jo...@mesosphere.io>.
> On March 6, 2015, 1:19 p.m., Joerg Schad wrote:
> > src/master/flags.hpp, line 383
> > <https://reviews.apache.org/r/29507/diff/4/?file=868836#file868836line383>
> >
> > Shouldn't this also be added to the documentation (i.e. http://mesos.apache.org/documentation/latest/configuration/)?
>
> Adam B wrote:
> Excellent point. Will update that in the next patch revision.
Thanks for fixing!
- Joerg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review75484
-----------------------------------------------------------
On May 14, 2015, 10:01 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated May 14, 2015, 10:01 a.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> docs/configuration.md 54c4e31
> docs/upgrades.md 2a15694
> src/master/constants.hpp c386eab
> src/master/constants.cpp 9ee17e9
> src/master/flags.hpp 996cf38
> src/master/flags.cpp 5798989
> src/master/master.cpp eaea79d
> src/messages/messages.proto 19e2444
> src/slave/constants.hpp df02043
> src/slave/constants.cpp 07f699a
> src/slave/slave.hpp b62ed7b
> src/slave/slave.cpp 132f83e
> src/tests/partition_tests.cpp f7ee3ab
> src/tests/slave_recovery_tests.cpp c036e9c
> src/tests/slave_tests.cpp acae497
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Joerg Schad <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review75484
-----------------------------------------------------------
src/master/flags.hpp
<https://reviews.apache.org/r/29507/#comment122562>
Shouldn't this also be added to the documentation (i.e. http://mesos.apache.org/documentation/latest/configuration/)?
- Joerg Schad
On Feb. 19, 2015, 8:10 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2015, 8:10 a.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/constants.hpp ad3fe81
> src/master/constants.cpp d3d0f71
> src/master/flags.hpp 51a6059
> src/master/master.cpp f10a3cf
> src/messages/messages.proto 58484ae
> src/slave/constants.hpp 12d6e92
> src/slave/constants.cpp 7868bef
> src/slave/slave.hpp 91dae10
> src/slave/slave.cpp aec9525
> src/tests/fault_tolerance_tests.cpp efa5c57
> src/tests/partition_tests.cpp eb16a58
> src/tests/slave_recovery_tests.cpp 8210c52
> src/tests/slave_tests.cpp 153d9d6
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/
-----------------------------------------------------------
(Updated Feb. 19, 2015, 12:10 a.m.)
Review request for mesos, Ben Mahler and Niklas Nielsen.
Changes
-------
Merged in subsequent patch to make slave use master's ping timeout from SlaveRe[re]gisteredMessage, plus unit tests.
Bugs: MESOS-2110
https://issues.apache.org/jira/browse/MESOS-2110
Repository: mesos
Description (updated)
-------
Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
These can be extended if slaves are expected/allowed to be down for
longer than a minute or two.
Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
Beware that this affects recovery from network timeouts as well as
actual slave node/process failover.
Diffs (updated)
-----
src/master/constants.hpp ad3fe81
src/master/constants.cpp d3d0f71
src/master/flags.hpp 51a6059
src/master/master.cpp f10a3cf
src/messages/messages.proto 58484ae
src/slave/constants.hpp 12d6e92
src/slave/constants.cpp 7868bef
src/slave/slave.hpp 91dae10
src/slave/slave.cpp aec9525
src/tests/fault_tolerance_tests.cpp efa5c57
src/tests/partition_tests.cpp eb16a58
src/tests/slave_recovery_tests.cpp 8210c52
src/tests/slave_tests.cpp 153d9d6
Diff: https://reviews.apache.org/r/29507/diff/
Testing (updated)
-------
Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
Ran unit tests with shorter non-default values for ping timeouts.
`make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
Thanks,
Adam B
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/
-----------------------------------------------------------
(Updated Feb. 18, 2015, 4:33 p.m.)
Review request for mesos and Niklas Nielsen.
Changes
-------
Rebased, in preparation for part 2 (slave-side timeouts).
Bugs: MESOS-2110
https://issues.apache.org/jira/browse/MESOS-2110
Repository: mesos
Description (updated)
-------
Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
These can be extended if slaves are expected/allowed to be down for
longer than a minute or two.
Beware that this affects recovery from network timeouts as well as
actual slave node/process failover.
Diffs (updated)
-----
src/master/constants.hpp c386eab
src/master/constants.cpp 9ee17e9
src/master/flags.hpp 6c18a1a
src/master/master.cpp f4b6463
src/slave/constants.hpp 761cfaf
src/slave/constants.cpp 83d9fc1
src/slave/slave.cpp a8b2621
src/tests/fault_tolerance_tests.cpp f927d4a
src/tests/partition_tests.cpp fea7801
src/tests/slave_recovery_tests.cpp 7e2e63d
src/tests/slave_tests.cpp e7e2af6
Diff: https://reviews.apache.org/r/29507/diff/
Testing (updated)
-------
Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
Ran unit tests with shorter non-default values for ping timeouts.
Thanks,
Adam B
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review67183
-----------------------------------------------------------
Patch looks great!
Reviews applied: [29507]
All tests passed.
- Mesos ReviewBot
On Jan. 8, 2015, 8:44 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Jan. 8, 2015, 8:44 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to replace the existing (still default)
> SLAVE_PING_TIMEOUT (15secs) and MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/constants.hpp c386eab
> src/master/constants.cpp 9ee17e9
> src/master/flags.hpp f5c8d2a
> src/master/master.cpp d6651e2
> src/slave/constants.hpp fd1c1ab
> src/slave/constants.cpp 2a99b11
> src/slave/slave.cpp 50b5781
> src/tests/fault_tolerance_tests.cpp 5763486
> src/tests/partition_tests.cpp fea7801
> src/tests/slave_recovery_tests.cpp cd4a398
> src/tests/slave_tests.cpp c50cbc7
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Updated unit tests to use shorter non-default values for ping timeouts.
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Ben Mahler <be...@gmail.com>.
> On Feb. 18, 2015, 7:38 p.m., Niklas Nielsen wrote:
> > Let's get tests wired up before committing this :)
>
> Adam B wrote:
> Sure thing. Adding tests in my subsequent patch where we will pass the master's timeout values on to the slave. Will post that very soon.
>
> Ben Mahler wrote:
> Can you do it in one patch? This patch in isolation looks a bit dangerous per our conversation above.
>
> Also, please carefully consider whether your approach will be safe to do in a single version. i.e. What happens when there are old slaves running against a new master? And vice versa.
>
> Adam B wrote:
> Easy enough. Added the second patch to this one. Most of the new changes are in messages.proto, slave.hpp/cpp, and partition_tests.cpp, with a few lines in master.cpp to set the new message fields.
>
> Certainly safe to upgrade if the new flags stay at their default value. Also, with new slaves talking to an old master, the master will still use the defaults, hence so will the slaves.
>
> But old slaves running against a new master with a longer timeout will try to reregister unnecessarily early, so you may want to guarantee that all/most of the slaves have been upgraded before setting these flags, otherwise a large cluster suddenly waking up from a network split would see a lot of unnecessary reregistration attempts. The old behavior in this scenario was that the slaves would reregister after the same default timeout after the network split, and the master would consider them shutdown after the same timeout.
> If that last scenario is a problem, maybe the better approach is for each slave to specify its own ping timeout, and then the master can use these values with each slave's SlaveObserver. Then we can just require the master(s) to be upgraded first, as is typical.
>
> Adam B wrote:
> I'm inclined to think that we shouldn't worry too much about the unlikely scenario of a network split in the middle of a rolling upgrade to 0.23 where longer ping timeouts are simultaneously being applied. Procedure should be: upgrade (most of) the cluster, then restart the master(s) with the new longer ping timeout value. I can add a Note to upgrades.md if you like. How does that sound to you @bmahler ?
> If there are no objections, I'll rebase the patch and update the configuration and upgrades docs.
Yeah that sounds fine to me (might be prudent to call it out in the upgrade doc or the changelog).
Mind reaching out to me directly when this is ready for more reviewing? :)
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review72992
-----------------------------------------------------------
On Feb. 19, 2015, 8:10 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2015, 8:10 a.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/constants.hpp ad3fe81
> src/master/constants.cpp d3d0f71
> src/master/flags.hpp 51a6059
> src/master/master.cpp f10a3cf
> src/messages/messages.proto 58484ae
> src/slave/constants.hpp 12d6e92
> src/slave/constants.cpp 7868bef
> src/slave/slave.hpp 91dae10
> src/slave/slave.cpp aec9525
> src/tests/fault_tolerance_tests.cpp efa5c57
> src/tests/partition_tests.cpp eb16a58
> src/tests/slave_recovery_tests.cpp 8210c52
> src/tests/slave_tests.cpp 153d9d6
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
> On Feb. 18, 2015, 11:38 a.m., Niklas Nielsen wrote:
> > Let's get tests wired up before committing this :)
>
> Adam B wrote:
> Sure thing. Adding tests in my subsequent patch where we will pass the master's timeout values on to the slave. Will post that very soon.
>
> Ben Mahler wrote:
> Can you do it in one patch? This patch in isolation looks a bit dangerous per our conversation above.
>
> Also, please carefully consider whether your approach will be safe to do in a single version. i.e. What happens when there are old slaves running against a new master? And vice versa.
>
> Adam B wrote:
> Easy enough. Added the second patch to this one. Most of the new changes are in messages.proto, slave.hpp/cpp, and partition_tests.cpp, with a few lines in master.cpp to set the new message fields.
>
> Certainly safe to upgrade if the new flags stay at their default value. Also, with new slaves talking to an old master, the master will still use the defaults, hence so will the slaves.
>
> But old slaves running against a new master with a longer timeout will try to reregister unnecessarily early, so you may want to guarantee that all/most of the slaves have been upgraded before setting these flags, otherwise a large cluster suddenly waking up from a network split would see a lot of unnecessary reregistration attempts. The old behavior in this scenario was that the slaves would reregister after the same default timeout after the network split, and the master would consider them shutdown after the same timeout.
> If that last scenario is a problem, maybe the better approach is for each slave to specify its own ping timeout, and then the master can use these values with each slave's SlaveObserver. Then we can just require the master(s) to be upgraded first, as is typical.
I'm inclined to think that we shouldn't worry too much about the unlikely scenario of a network split in the middle of a rolling upgrade to 0.23 where longer ping timeouts are simultaneously being applied. Procedure should be: upgrade (most of) the cluster, then restart the master(s) with the new longer ping timeout value. I can add a Note to upgrades.md if you like. How does that sound to you @bmahler ?
If there are no objections, I'll rebase the patch and update the configuration and upgrades docs.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review72992
-----------------------------------------------------------
On Feb. 19, 2015, 12:10 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2015, 12:10 a.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/constants.hpp ad3fe81
> src/master/constants.cpp d3d0f71
> src/master/flags.hpp 51a6059
> src/master/master.cpp f10a3cf
> src/messages/messages.proto 58484ae
> src/slave/constants.hpp 12d6e92
> src/slave/constants.cpp 7868bef
> src/slave/slave.hpp 91dae10
> src/slave/slave.cpp aec9525
> src/tests/fault_tolerance_tests.cpp efa5c57
> src/tests/partition_tests.cpp eb16a58
> src/tests/slave_recovery_tests.cpp 8210c52
> src/tests/slave_tests.cpp 153d9d6
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
> On Feb. 18, 2015, 11:38 a.m., Niklas Nielsen wrote:
> > Let's get tests wired up before committing this :)
>
> Adam B wrote:
> Sure thing. Adding tests in my subsequent patch where we will pass the master's timeout values on to the slave. Will post that very soon.
>
> Ben Mahler wrote:
> Can you do it in one patch? This patch in isolation looks a bit dangerous per our conversation above.
>
> Also, please carefully consider whether your approach will be safe to do in a single version. i.e. What happens when there are old slaves running against a new master? And vice versa.
>
> Adam B wrote:
> Easy enough. Added the second patch to this one. Most of the new changes are in messages.proto, slave.hpp/cpp, and partition_tests.cpp, with a few lines in master.cpp to set the new message fields.
>
> Certainly safe to upgrade if the new flags stay at their default value. Also, with new slaves talking to an old master, the master will still use the defaults, hence so will the slaves.
>
> But old slaves running against a new master with a longer timeout will try to reregister unnecessarily early, so you may want to guarantee that all/most of the slaves have been upgraded before setting these flags, otherwise a large cluster suddenly waking up from a network split would see a lot of unnecessary reregistration attempts. The old behavior in this scenario was that the slaves would reregister after the same default timeout after the network split, and the master would consider them shutdown after the same timeout.
> If that last scenario is a problem, maybe the better approach is for each slave to specify its own ping timeout, and then the master can use these values with each slave's SlaveObserver. Then we can just require the master(s) to be upgraded first, as is typical.
>
> Adam B wrote:
> I'm inclined to think that we shouldn't worry too much about the unlikely scenario of a network split in the middle of a rolling upgrade to 0.23 where longer ping timeouts are simultaneously being applied. Procedure should be: upgrade (most of) the cluster, then restart the master(s) with the new longer ping timeout value. I can add a Note to upgrades.md if you like. How does that sound to you @bmahler ?
> If there are no objections, I'll rebase the patch and update the configuration and upgrades docs.
>
> Ben Mahler wrote:
> Yeah that sounds fine to me (might be prudent to call it out in the upgrade doc or the changelog).
>
> Mind reaching out to me directly when this is ready for more reviewing? :)
Absolutely. Thanks. :)
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review72992
-----------------------------------------------------------
On Feb. 19, 2015, 12:10 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2015, 12:10 a.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/constants.hpp ad3fe81
> src/master/constants.cpp d3d0f71
> src/master/flags.hpp 51a6059
> src/master/master.cpp f10a3cf
> src/messages/messages.proto 58484ae
> src/slave/constants.hpp 12d6e92
> src/slave/constants.cpp 7868bef
> src/slave/slave.hpp 91dae10
> src/slave/slave.cpp aec9525
> src/tests/fault_tolerance_tests.cpp efa5c57
> src/tests/partition_tests.cpp eb16a58
> src/tests/slave_recovery_tests.cpp 8210c52
> src/tests/slave_tests.cpp 153d9d6
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
> On Feb. 18, 2015, 11:38 a.m., Niklas Nielsen wrote:
> > Let's get tests wired up before committing this :)
>
> Adam B wrote:
> Sure thing. Adding tests in my subsequent patch where we will pass the master's timeout values on to the slave. Will post that very soon.
>
> Ben Mahler wrote:
> Can you do it in one patch? This patch in isolation looks a bit dangerous per our conversation above.
>
> Also, please carefully consider whether your approach will be safe to do in a single version. i.e. What happens when there are old slaves running against a new master? And vice versa.
Easy enough. Added the second patch to this one. Most of the new changes are in messages.proto, slave.hpp/cpp, and partition_tests.cpp, with a few lines in master.cpp to set the new message fields.
Certainly safe to upgrade if the new flags stay at their default value. Also, with new slaves talking to an old master, the master will still use the defaults, hence so will the slaves.
But old slaves running against a new master with a longer timeout will try to reregister unnecessarily early, so you may want to guarantee that all/most of the slaves have been upgraded before setting these flags, otherwise a large cluster suddenly waking up from a network split would see a lot of unnecessary reregistration attempts. The old behavior in this scenario was that the slaves would reregister after the same default timeout after the network split, and the master would consider them shutdown after the same timeout.
If that last scenario is a problem, maybe the better approach is for each slave to specify its own ping timeout, and then the master can use these values with each slave's SlaveObserver. Then we can just require the master(s) to be upgraded first, as is typical.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review72992
-----------------------------------------------------------
On Feb. 19, 2015, 12:10 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2015, 12:10 a.m.)
>
>
> Review request for mesos, Ben Mahler and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/constants.hpp ad3fe81
> src/master/constants.cpp d3d0f71
> src/master/flags.hpp 51a6059
> src/master/master.cpp f10a3cf
> src/messages/messages.proto 58484ae
> src/slave/constants.hpp 12d6e92
> src/slave/constants.cpp 7868bef
> src/slave/slave.hpp 91dae10
> src/slave/slave.cpp aec9525
> src/tests/fault_tolerance_tests.cpp efa5c57
> src/tests/partition_tests.cpp eb16a58
> src/tests/slave_recovery_tests.cpp 8210c52
> src/tests/slave_tests.cpp 153d9d6
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
> `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
> On Feb. 18, 2015, 11:38 a.m., Niklas Nielsen wrote:
> > Let's get tests wired up before committing this :)
Sure thing. Adding tests in my subsequent patch where we will pass the master's timeout values on to the slave. Will post that very soon.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review72992
-----------------------------------------------------------
On Jan. 8, 2015, 12:44 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Jan. 8, 2015, 12:44 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to replace the existing (still default)
> SLAVE_PING_TIMEOUT (15secs) and MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/constants.hpp c386eab
> src/master/constants.cpp 9ee17e9
> src/master/flags.hpp f5c8d2a
> src/master/master.cpp d6651e2
> src/slave/constants.hpp fd1c1ab
> src/slave/constants.cpp 2a99b11
> src/slave/slave.cpp 50b5781
> src/tests/fault_tolerance_tests.cpp 5763486
> src/tests/partition_tests.cpp fea7801
> src/tests/slave_recovery_tests.cpp cd4a398
> src/tests/slave_tests.cpp c50cbc7
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Updated unit tests to use shorter non-default values for ping timeouts.
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Ben Mahler <be...@gmail.com>.
> On Feb. 18, 2015, 7:38 p.m., Niklas Nielsen wrote:
> > Let's get tests wired up before committing this :)
>
> Adam B wrote:
> Sure thing. Adding tests in my subsequent patch where we will pass the master's timeout values on to the slave. Will post that very soon.
Can you do it in one patch? This patch in isolation looks a bit dangerous per our conversation above.
Also, please carefully consider whether your approach will be safe to do in a single version. i.e. What happens when there are old slaves running against a new master? And vice versa.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review72992
-----------------------------------------------------------
On Feb. 19, 2015, 12:33 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Feb. 19, 2015, 12:33 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs)
> and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/constants.hpp c386eab
> src/master/constants.cpp 9ee17e9
> src/master/flags.hpp 6c18a1a
> src/master/master.cpp f4b6463
> src/slave/constants.hpp 761cfaf
> src/slave/constants.cpp 83d9fc1
> src/slave/slave.cpp a8b2621
> src/tests/fault_tolerance_tests.cpp f927d4a
> src/tests/partition_tests.cpp fea7801
> src/tests/slave_recovery_tests.cpp 7e2e63d
> src/tests/slave_tests.cpp e7e2af6
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Ran unit tests with shorter non-default values for ping timeouts.
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review72992
-----------------------------------------------------------
Let's get tests wired up before committing this :)
- Niklas Nielsen
On Jan. 8, 2015, 12:44 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Jan. 8, 2015, 12:44 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to replace the existing (still default)
> SLAVE_PING_TIMEOUT (15secs) and MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/constants.hpp c386eab
> src/master/constants.cpp 9ee17e9
> src/master/flags.hpp f5c8d2a
> src/master/master.cpp d6651e2
> src/slave/constants.hpp fd1c1ab
> src/slave/constants.cpp 2a99b11
> src/slave/slave.cpp 50b5781
> src/tests/fault_tolerance_tests.cpp 5763486
> src/tests/partition_tests.cpp fea7801
> src/tests/slave_recovery_tests.cpp cd4a398
> src/tests/slave_tests.cpp c50cbc7
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Updated unit tests to use shorter non-default values for ping timeouts.
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review67182
-----------------------------------------------------------
src/tests/partition_tests.cpp
<https://reviews.apache.org/r/29507/#comment111044>
I believe this only ever needed to be `slave_ping_timeout`, not `slave_ping_timeout * max_slave_ping_timeouts`, since we just want to advance the clock long enough for the SlaveObserver to send the next ping, not so long that the master considers the slave shutdown or so long that the slave tries to reregister after not receiving a ping.
- Adam B
On Jan. 8, 2015, 12:44 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Jan. 8, 2015, 12:44 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to replace the existing (still default)
> SLAVE_PING_TIMEOUT (15secs) and MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/constants.hpp c386eab
> src/master/constants.cpp 9ee17e9
> src/master/flags.hpp f5c8d2a
> src/master/master.cpp d6651e2
> src/slave/constants.hpp fd1c1ab
> src/slave/constants.cpp 2a99b11
> src/slave/slave.cpp 50b5781
> src/tests/fault_tolerance_tests.cpp 5763486
> src/tests/partition_tests.cpp fea7801
> src/tests/slave_recovery_tests.cpp cd4a398
> src/tests/slave_tests.cpp c50cbc7
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Updated unit tests to use shorter non-default values for ping timeouts.
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/
-----------------------------------------------------------
(Updated Jan. 8, 2015, 12:44 a.m.)
Review request for mesos and Niklas Nielsen.
Changes
-------
Responding to review feedback.
Bugs: MESOS-2110
https://issues.apache.org/jira/browse/MESOS-2110
Repository: mesos-git
Description
-------
Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
to mesos-master to replace the existing (still default)
SLAVE_PING_TIMEOUT (15secs) and MAX_SLAVE_PING_TIMEOUTS (5).
These can be extended if slaves are expected/allowed to be down for
longer than a minute or two.
Beware that this affects recovery from network timeouts as well as
actual slave node/process failover.
Diffs (updated)
-----
src/master/constants.hpp c386eab
src/master/constants.cpp 9ee17e9
src/master/flags.hpp f5c8d2a
src/master/master.cpp d6651e2
src/slave/constants.hpp fd1c1ab
src/slave/constants.cpp 2a99b11
src/slave/slave.cpp 50b5781
src/tests/fault_tolerance_tests.cpp 5763486
src/tests/partition_tests.cpp fea7801
src/tests/slave_recovery_tests.cpp cd4a398
src/tests/slave_tests.cpp c50cbc7
Diff: https://reviews.apache.org/r/29507/diff/
Testing
-------
Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
Updated unit tests to use shorter non-default values for ping timeouts.
Thanks,
Adam B
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review66461
-----------------------------------------------------------
Patch looks great!
Reviews applied: [29507]
All tests passed.
- Mesos ReviewBot
On Dec. 31, 2014, 9:38 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Dec. 31, 2014, 9:38 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2150
> https://issues.apache.org/jira/browse/MESOS-2150
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to replace the existing (still default)
> SLAVE_PING_TIMEOUT (15secs) and MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/flags.hpp f5c8d2a8cc7530bc8746935af9ea90af747cc111
> src/master/master.cpp d6651e299ddb73bfdc1b126c474075db6cda8acd
> src/tests/fault_tolerance_tests.cpp 5763486acb6d687b50c02c01ea00e1cfbea48421
> src/tests/mesos.cpp 3b98c69a604132be71a60fbbee4a47b51fe6956a
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_recovery_tests.cpp cd4a398ef680b5694cb6069b8e2ca4e2c05911d1
> src/tests/slave_tests.cpp c50cbc799d4793243f184f9fe628b69a020adc66
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Updated unit tests to use shorter non-default values for ping timeouts.
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
> On Jan. 6, 2015, 9:17 a.m., Niklas Nielsen wrote:
> > src/tests/mesos.cpp, line 129
> > <https://reviews.apache.org/r/29507/diff/1/?file=804648#file804648line129>
> >
> > Can we use a constant here instead (to make sure it follows future changes to the default value?)
The point of setting these values here was for them to be _different_ from the default value (3x5s vs. 5x15s). I could a) create constants that aren't used anywhere else yet, but might be wanted by some test in the future; b) divide the default by some constant factor, like what registry_store_timeout does; or c) just use let our unit tests use the default values. I'm leaning towards (c), since I don't think any of our tests care about the exact value, and any future test that does can override the default itself.
> On Jan. 6, 2015, 9:17 a.m., Niklas Nielsen wrote:
> > src/master/master.cpp, line 1111
> > <https://reviews.apache.org/r/29507/diff/1/?file=804646#file804646line1111>
> >
> > Can you expand a bit on this change?
Why certainly. I believe we have been logging the wrong value here, ever since --recovery_slave_removal_limit was introduced in https://reviews.apache.org/r/20097/diff/#. @bmahler correct me if I'm wrong.
Once the registrar has recovered, `Master::_recover` starts a `delay` timer for `flags.slave_reregister_timeout`, after which point it calls `Master::recoveredSlavesTimeout`, which is the method here that could exit with the message "After _X_ there were [an unacceptable number of] slaves recovered from the registry that did not reregister". This after `_recover` has already logged "Recovered ... slaves from the Registry; allowing [slave_reregister_timeout] for slaves to re-register".
The slave ping timeouts determine when the master considers a slave shutdown, sends SLAVE_LOST, and no longer accepts re-registrations; whereas the slave reregister timeout determines how soon after leader-election (and/or recovery) the master should expect slaves to reregister. This log message describes the latter scenario, so I corrected the value we were logging.
Make sense?
> On Jan. 6, 2015, 9:17 a.m., Niklas Nielsen wrote:
> > src/master/flags.hpp, line 406
> > <https://reviews.apache.org/r/29507/diff/1/?file=804645#file804645line406>
> >
> > Why uint32 and not int?
Mostly because the original default value constant is `src/master/constants.cpp:35:const uint32_t MAX_SLAVE_PING_TIMEOUTS = 5;`
My guess why _that_ was uint32? Well, any count variable (like the existing `uint32 SlaveObserver::timeout`) should never be negative and valid, so unsigned makes sense. I would think that one shouldn't need more than 255 timeouts (failed pings) before declaring a slave shutdown, so a uchar/byte would be sufficient, but maybe those bytes aren't worth saving, compared to having more bits to express a larger pseudo-infinity.
Now does the flag itself need to be uint32? Well, doing so puts the burden of size/sign validation on the flag-parsing code instead of wherever I end up doing the conversion.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review66859
-----------------------------------------------------------
On Dec. 31, 2014, 1:38 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Dec. 31, 2014, 1:38 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2150
> https://issues.apache.org/jira/browse/MESOS-2150
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to replace the existing (still default)
> SLAVE_PING_TIMEOUT (15secs) and MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/flags.hpp f5c8d2a8cc7530bc8746935af9ea90af747cc111
> src/master/master.cpp d6651e299ddb73bfdc1b126c474075db6cda8acd
> src/tests/fault_tolerance_tests.cpp 5763486acb6d687b50c02c01ea00e1cfbea48421
> src/tests/mesos.cpp 3b98c69a604132be71a60fbbee4a47b51fe6956a
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_recovery_tests.cpp cd4a398ef680b5694cb6069b8e2ca4e2c05911d1
> src/tests/slave_tests.cpp c50cbc799d4793243f184f9fe628b69a020adc66
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Updated unit tests to use shorter non-default values for ping timeouts.
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Michael Park <mc...@gmail.com>.
> On Jan. 6, 2015, 5:17 p.m., Niklas Nielsen wrote:
> > src/tests/mesos.cpp, line 129
> > <https://reviews.apache.org/r/29507/diff/1/?file=804648#file804648line129>
> >
> > Can we use a constant here instead (to make sure it follows future changes to the default value?)
>
> Adam B wrote:
> The point of setting these values here was for them to be _different_ from the default value (3x5s vs. 5x15s). I could a) create constants that aren't used anywhere else yet, but might be wanted by some test in the future; b) divide the default by some constant factor, like what registry_store_timeout does; or c) just use let our unit tests use the default values. I'm leaning towards (c), since I don't think any of our tests care about the exact value, and any future test that does can override the default itself.
I'm +1 for (c) as well, but I don't understand why you would've changed this in the first place then?
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review66859
-----------------------------------------------------------
On Dec. 31, 2014, 9:38 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Dec. 31, 2014, 9:38 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2150
> https://issues.apache.org/jira/browse/MESOS-2150
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to replace the existing (still default)
> SLAVE_PING_TIMEOUT (15secs) and MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/flags.hpp f5c8d2a8cc7530bc8746935af9ea90af747cc111
> src/master/master.cpp d6651e299ddb73bfdc1b126c474075db6cda8acd
> src/tests/fault_tolerance_tests.cpp 5763486acb6d687b50c02c01ea00e1cfbea48421
> src/tests/mesos.cpp 3b98c69a604132be71a60fbbee4a47b51fe6956a
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_recovery_tests.cpp cd4a398ef680b5694cb6069b8e2ca4e2c05911d1
> src/tests/slave_tests.cpp c50cbc799d4793243f184f9fe628b69a020adc66
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Updated unit tests to use shorter non-default values for ping timeouts.
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
> On Jan. 6, 2015, 9:17 a.m., Niklas Nielsen wrote:
> > src/tests/mesos.cpp, line 129
> > <https://reviews.apache.org/r/29507/diff/1/?file=804648#file804648line129>
> >
> > Can we use a constant here instead (to make sure it follows future changes to the default value?)
>
> Adam B wrote:
> The point of setting these values here was for them to be _different_ from the default value (3x5s vs. 5x15s). I could a) create constants that aren't used anywhere else yet, but might be wanted by some test in the future; b) divide the default by some constant factor, like what registry_store_timeout does; or c) just use let our unit tests use the default values. I'm leaning towards (c), since I don't think any of our tests care about the exact value, and any future test that does can override the default itself.
>
> Michael Park wrote:
> I'm +1 for (c) as well, but I don't understand why you would've changed this in the first place then?
I think I just changed it for personal debugging/sanity-testing. Removing these lines now.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review66859
-----------------------------------------------------------
On Dec. 31, 2014, 1:38 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Dec. 31, 2014, 1:38 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2150
> https://issues.apache.org/jira/browse/MESOS-2150
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to replace the existing (still default)
> SLAVE_PING_TIMEOUT (15secs) and MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/flags.hpp f5c8d2a8cc7530bc8746935af9ea90af747cc111
> src/master/master.cpp d6651e299ddb73bfdc1b126c474075db6cda8acd
> src/tests/fault_tolerance_tests.cpp 5763486acb6d687b50c02c01ea00e1cfbea48421
> src/tests/mesos.cpp 3b98c69a604132be71a60fbbee4a47b51fe6956a
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_recovery_tests.cpp cd4a398ef680b5694cb6069b8e2ca4e2c05911d1
> src/tests/slave_tests.cpp c50cbc799d4793243f184f9fe628b69a020adc66
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Updated unit tests to use shorter non-default values for ping timeouts.
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review66859
-----------------------------------------------------------
Looks good! Have a couple of small questions
src/master/flags.hpp
<https://reviews.apache.org/r/29507/#comment110533>
Why uint32 and not int?
src/master/master.cpp
<https://reviews.apache.org/r/29507/#comment110532>
Can you expand a bit on this change?
src/master/master.cpp
<https://reviews.apache.org/r/29507/#comment110531>
According to the wrapping rules, we should newline each argument here no?
src/tests/mesos.cpp
<https://reviews.apache.org/r/29507/#comment110530>
Can we use a constant here instead (to make sure it follows future changes to the default value?)
src/tests/partition_tests.cpp
<https://reviews.apache.org/r/29507/#comment110529>
s/> >/>>/
src/tests/slave_recovery_tests.cpp
<https://reviews.apache.org/r/29507/#comment110528>
s/> >/>>/
- Niklas Nielsen
On Dec. 31, 2014, 1:38 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Dec. 31, 2014, 1:38 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2150
> https://issues.apache.org/jira/browse/MESOS-2150
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to replace the existing (still default)
> SLAVE_PING_TIMEOUT (15secs) and MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/flags.hpp f5c8d2a8cc7530bc8746935af9ea90af747cc111
> src/master/master.cpp d6651e299ddb73bfdc1b126c474075db6cda8acd
> src/tests/fault_tolerance_tests.cpp 5763486acb6d687b50c02c01ea00e1cfbea48421
> src/tests/mesos.cpp 3b98c69a604132be71a60fbbee4a47b51fe6956a
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_recovery_tests.cpp cd4a398ef680b5694cb6069b8e2ca4e2c05911d1
> src/tests/slave_tests.cpp c50cbc799d4793243f184f9fe628b69a020adc66
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Updated unit tests to use shorter non-default values for ping timeouts.
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Ben Mahler <be...@gmail.com>.
> On Jan. 7, 2015, 10:50 p.m., Ben Mahler wrote:
> > src/master/flags.hpp, lines 361-373
> > <https://reviews.apache.org/r/29507/diff/1/?file=804645#file804645line361>
> >
> > Do you mind changing the names now that they're merely defaults?
> >
> > s/SLAVE_PING_TIMEOUT/DEFAULT_SLAVE_PING_TIMEOUT/
> > s/MAX_SLAVE_PING_TIMEOUTS/DEFAULT_MAX_SLAVE_PING_TIMEOUTS/
> >
> > Once you do this, it will become apparent there is more needed in this change. In particular, all the slave logic that depends on MASTER_PING_TIMEOUT() is broken now! Making it configurable will require some more thought.
>
> Adam B wrote:
> Renamed to make the DEFAULT nature explicit.
>
> I would argue that with this change as is, the slave side of the ping timeout isn't much more broken than it already was. As I alluded to in the MESOS-2110 JIRA description, the slave uses the timeout to force a leader-detection & re-registration, which is only loosely related to what the master is doing with the timeout (declaring the slave shutdown). Even without my changes, the Timers on master/slave could be out of sync and the two timeout events aren't guaranteed to happen in a particular order. If the master were to timeout first, and the slave did eventually detect a new leader, the slave would still be told to shutdown and its re-registration would fail. If the slave timed out first, it would try to re-register with the currently verified leader (could be the same), and the master's SlaveObserver would keep trying to ping the slave and potentially eventually shut it down. That's why I would argue that this patch is fine on its own, and a subsequent patch (same JIRA?) sh
ould clean up the slave's ping timeout behavior.
>
> `src/master/constants.hpp` has the following note:
> ```
> // NOTE: The slave uses these PING constants to determine when
> // the master has stopped sending pings. If these are made
> // configurable, then we'll need to rely on upper/lower bounds
> // to ensure that the slave is not unnecessarily triggering
> // re-registrations.
> ```
> If the master flags are configurable, there's currently no way for the slave to know what value the current leading master is using, especially if the slave is disconnected at the time. After reading through the comments in MESOS-1529 (which added the slave-side ping timeout), I'm thinking that maybe the master->slave registered (and PING?) messages could embed the current master's actual ping timeout. Then the pingTimers started from Slave::registered() and Slave::ping() could use that value (same as master's) to prevent unnecessary reregistration due to a timeout mismatch.
>
> Thoughts, suggestions?
> I would argue that with this change as is, the slave side of the ping timeout isn't much more broken than it already was.
Hm.. I think you misunderstood the intent of the slave using this timeout (it's not trying to prevent the things you discussed, please clarify if you think it's causing some issues, I didn't quite understand what you're pointing out).
These timers are in place to guarantee that if the master removes a slave (stops health checking it), then the slave will eventually terminate. Without this timeout, it's possible for the master -> slave removal messages to drop while the slave -> master socket remains intact. At this point, (without the timeout) the slave would remain running potentially forever (unless a message (e.g. status update) is sent to the master for the master to detect the zombied slave).
The slave also uses the pings to prevent a slave from getting "stuck" in a situation where the master considers the slave disconnected, but the slave considers itself connected. This can happen via a one-way socket closure from master -> slave. Note that the health checking in the master continues while the slave is disconnected.
The comments here should help:
```
void Slave::ping(const UPID& from, bool connected)
{
VLOG(1) << "Received ping from " << from;
if (!connected && state == RUNNING) {
// This could happen if there is a one way partition between
// the master and slave, causing the master to get an exited
// event and marking the slave disconnected but the slave
// thinking it is still connected. Force a re-registration with
// the master to reconcile.
LOG(INFO) << "Master marked the slave as disconnected but the slave"
<< " considers itself registered! Forcing re-registration.";
detection.discard();
}
// If we don't get a ping from the master, trigger a
// re-registration. This can occur when the master no
// longer considers the slave to be registered, so it is
// essential for the slave to attempt a re-registration
// when this occurs.
Clock::cancel(pingTimer);
pingTimer = delay(
MASTER_PING_TIMEOUT(),
self(),
&Slave::pingTimeout,
detection);
send(from, PongSlaveMessage());
}
```
Based on all of this, it's really important to understand the implications of the slave under and over estimating the timeout on the master side. For example, if the slave under-estimates and always triggers re-registration, that would be problematic on large clusters (40k slaves / 75 seconds = 500+ re-registrations / second in the steady state!). That's why a TODO mentioned using upper/lower bounds for this.
- Ben
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review67113
-----------------------------------------------------------
On Jan. 8, 2015, 8:44 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Jan. 8, 2015, 8:44 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to replace the existing (still default)
> SLAVE_PING_TIMEOUT (15secs) and MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/constants.hpp c386eab
> src/master/constants.cpp 9ee17e9
> src/master/flags.hpp f5c8d2a
> src/master/master.cpp d6651e2
> src/slave/constants.hpp fd1c1ab
> src/slave/constants.cpp 2a99b11
> src/slave/slave.cpp 50b5781
> src/tests/fault_tolerance_tests.cpp 5763486
> src/tests/partition_tests.cpp fea7801
> src/tests/slave_recovery_tests.cpp cd4a398
> src/tests/slave_tests.cpp c50cbc7
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Updated unit tests to use shorter non-default values for ping timeouts.
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
> On Jan. 7, 2015, 2:50 p.m., Ben Mahler wrote:
> > src/master/flags.hpp, lines 361-373
> > <https://reviews.apache.org/r/29507/diff/1/?file=804645#file804645line361>
> >
> > Do you mind changing the names now that they're merely defaults?
> >
> > s/SLAVE_PING_TIMEOUT/DEFAULT_SLAVE_PING_TIMEOUT/
> > s/MAX_SLAVE_PING_TIMEOUTS/DEFAULT_MAX_SLAVE_PING_TIMEOUTS/
> >
> > Once you do this, it will become apparent there is more needed in this change. In particular, all the slave logic that depends on MASTER_PING_TIMEOUT() is broken now! Making it configurable will require some more thought.
Renamed to make the DEFAULT nature explicit.
I would argue that with this change as is, the slave side of the ping timeout isn't much more broken than it already was. As I alluded to in the MESOS-2110 JIRA description, the slave uses the timeout to force a leader-detection & re-registration, which is only loosely related to what the master is doing with the timeout (declaring the slave shutdown). Even without my changes, the Timers on master/slave could be out of sync and the two timeout events aren't guaranteed to happen in a particular order. If the master were to timeout first, and the slave did eventually detect a new leader, the slave would still be told to shutdown and its re-registration would fail. If the slave timed out first, it would try to re-register with the currently verified leader (could be the same), and the master's SlaveObserver would keep trying to ping the slave and potentially eventually shut it down. That's why I would argue that this patch is fine on its own, and a subsequent patch (same JIRA?) should c
lean up the slave's ping timeout behavior.
`src/master/constants.hpp` has the following note:
```
// NOTE: The slave uses these PING constants to determine when
// the master has stopped sending pings. If these are made
// configurable, then we'll need to rely on upper/lower bounds
// to ensure that the slave is not unnecessarily triggering
// re-registrations.
```
If the master flags are configurable, there's currently no way for the slave to know what value the current leading master is using, especially if the slave is disconnected at the time. After reading through the comments in MESOS-1529 (which added the slave-side ping timeout), I'm thinking that maybe the master->slave registered (and PING?) messages could embed the current master's actual ping timeout. Then the pingTimers started from Slave::registered() and Slave::ping() could use that value (same as master's) to prevent unnecessary reregistration due to a timeout mismatch.
Thoughts, suggestions?
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review67113
-----------------------------------------------------------
On Dec. 31, 2014, 1:38 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Dec. 31, 2014, 1:38 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2150
> https://issues.apache.org/jira/browse/MESOS-2150
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to replace the existing (still default)
> SLAVE_PING_TIMEOUT (15secs) and MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/flags.hpp f5c8d2a8cc7530bc8746935af9ea90af747cc111
> src/master/master.cpp d6651e299ddb73bfdc1b126c474075db6cda8acd
> src/tests/fault_tolerance_tests.cpp 5763486acb6d687b50c02c01ea00e1cfbea48421
> src/tests/mesos.cpp 3b98c69a604132be71a60fbbee4a47b51fe6956a
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_recovery_tests.cpp cd4a398ef680b5694cb6069b8e2ca4e2c05911d1
> src/tests/slave_tests.cpp c50cbc799d4793243f184f9fe628b69a020adc66
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Updated unit tests to use shorter non-default values for ping timeouts.
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Jan. 7, 2015, 2:50 p.m., Ben Mahler wrote:
> > src/master/flags.hpp, lines 361-373
> > <https://reviews.apache.org/r/29507/diff/1/?file=804645#file804645line361>
> >
> > Do you mind changing the names now that they're merely defaults?
> >
> > s/SLAVE_PING_TIMEOUT/DEFAULT_SLAVE_PING_TIMEOUT/
> > s/MAX_SLAVE_PING_TIMEOUTS/DEFAULT_MAX_SLAVE_PING_TIMEOUTS/
> >
> > Once you do this, it will become apparent there is more needed in this change. In particular, all the slave logic that depends on MASTER_PING_TIMEOUT() is broken now! Making it configurable will require some more thought.
>
> Adam B wrote:
> Renamed to make the DEFAULT nature explicit.
>
> I would argue that with this change as is, the slave side of the ping timeout isn't much more broken than it already was. As I alluded to in the MESOS-2110 JIRA description, the slave uses the timeout to force a leader-detection & re-registration, which is only loosely related to what the master is doing with the timeout (declaring the slave shutdown). Even without my changes, the Timers on master/slave could be out of sync and the two timeout events aren't guaranteed to happen in a particular order. If the master were to timeout first, and the slave did eventually detect a new leader, the slave would still be told to shutdown and its re-registration would fail. If the slave timed out first, it would try to re-register with the currently verified leader (could be the same), and the master's SlaveObserver would keep trying to ping the slave and potentially eventually shut it down. That's why I would argue that this patch is fine on its own, and a subsequent patch (same JIRA?) sh
ould clean up the slave's ping timeout behavior.
>
> `src/master/constants.hpp` has the following note:
> ```
> // NOTE: The slave uses these PING constants to determine when
> // the master has stopped sending pings. If these are made
> // configurable, then we'll need to rely on upper/lower bounds
> // to ensure that the slave is not unnecessarily triggering
> // re-registrations.
> ```
> If the master flags are configurable, there's currently no way for the slave to know what value the current leading master is using, especially if the slave is disconnected at the time. After reading through the comments in MESOS-1529 (which added the slave-side ping timeout), I'm thinking that maybe the master->slave registered (and PING?) messages could embed the current master's actual ping timeout. Then the pingTimers started from Slave::registered() and Slave::ping() could use that value (same as master's) to prevent unnecessary reregistration due to a timeout mismatch.
>
> Thoughts, suggestions?
>
> Ben Mahler wrote:
> > I would argue that with this change as is, the slave side of the ping timeout isn't much more broken than it already was.
>
> Hm.. I think you misunderstood the intent of the slave using this timeout (it's not trying to prevent the things you discussed, please clarify if you think it's causing some issues, I didn't quite understand what you're pointing out).
>
> These timers are in place to guarantee that if the master removes a slave (stops health checking it), then the slave will eventually terminate. Without this timeout, it's possible for the master -> slave removal messages to drop while the slave -> master socket remains intact. At this point, (without the timeout) the slave would remain running potentially forever (unless a message (e.g. status update) is sent to the master for the master to detect the zombied slave).
>
> The slave also uses the pings to prevent a slave from getting "stuck" in a situation where the master considers the slave disconnected, but the slave considers itself connected. This can happen via a one-way socket closure from master -> slave. Note that the health checking in the master continues while the slave is disconnected.
>
> The comments here should help:
>
> ```
> void Slave::ping(const UPID& from, bool connected)
> {
> VLOG(1) << "Received ping from " << from;
>
> if (!connected && state == RUNNING) {
> // This could happen if there is a one way partition between
> // the master and slave, causing the master to get an exited
> // event and marking the slave disconnected but the slave
> // thinking it is still connected. Force a re-registration with
> // the master to reconcile.
> LOG(INFO) << "Master marked the slave as disconnected but the slave"
> << " considers itself registered! Forcing re-registration.";
> detection.discard();
> }
>
> // If we don't get a ping from the master, trigger a
> // re-registration. This can occur when the master no
> // longer considers the slave to be registered, so it is
> // essential for the slave to attempt a re-registration
> // when this occurs.
> Clock::cancel(pingTimer);
>
> pingTimer = delay(
> MASTER_PING_TIMEOUT(),
> self(),
> &Slave::pingTimeout,
> detection);
>
> send(from, PongSlaveMessage());
> }
> ```
>
> Based on all of this, it's really important to understand the implications of the slave under and over estimating the timeout on the master side. For example, if the slave under-estimates and always triggers re-registration, that would be problematic on large clusters (40k slaves / 75 seconds = 500+ re-registrations / second in the steady state!). That's why a TODO mentioned using upper/lower bounds for this.
>
> Adam B wrote:
> Thank you for reviewing/discussing. I hadn't thought much before about the one-way socket closure scenario, but it doesn't drastically change my perspective. As you mention, it's important to consider the implications of the slave misestimating the timeout, especially with a configurable timeout where a master and slave could be at different versions/configurations during a rolling upgrade. There must be one source of truth for this timeout, rather than allowing the slave to "estimate" or have its own timeout configuration. So I still propose that the master should pass `master_ping_timeout` to the slave via SlaveRe[re]gisteredMessage, so that any slave that has ever considered itself registered/RUNNING knows the master's actual value and will wait at least as long as the master, no matter if/when/how the socket closes. We could also pass the timeout in the PingSlaveMessage if we're paranoid about the master changing its ping timeout value without triggering a reregistration. I
f you aren't opposed, I can put together a patch and some tests to demonstrate.
>
> Even with the master communicating its timeout so that both sides use the same value, at the point of a two-way network split whichever side last received a Ping/Pong will start its timer later and consequently time out later due to the non-zero communication latency. In the event of a one-way socket closure where the slave can still reach the master, a successful Pong will always follow a successful Ping, so the slave will time out first.
> If the master times out first, it will send a ShutdownSlaveMessage, remove the slave, and terminate the SlaveObserver, never trying to contact the slave again. The slave may continue trying to send status updates, etc. in the meantime (which might go through in a one-way disconnect), but the slave will eventually timeout, disconnect itself (so it stops sending messages), and consult the MasterDetector for a leader to reregister with. If the master cannot reach the slave, it is likely that the ZK quorum cannot reach it either, but if the slave can reach the ZK quorum, or is configured with a static master, it will indeed try to re-authenticate/register, using a backoff for registration, but not for authentication (TODO).
> If the slave times out first, it will unnecessarily try to reregister/authenticate even though the master thinks it is still registered. If the master continues to send messages to the slave, they will all be dropped with a warning logged, except for Ping messages which will try to reply with a Pong. If the reregister/authenticate/pong messages from the slave cannot reach the master, the master will eventually remove the slave. If the slave is able to reregister before the master removes it (i.e. brief network connectivity issue), all will go back to normal, but the whole headache could have been avoided if the slave had timed out after the master.
> To try to enforce ordering (master always times out first), we could have the master timeout after `slave_ping_timeout*max_slave_ping_timeouts` and have the slave timeout after `slave_ping_timeout*(max_slave_ping_timeouts+1)`, passing these values from the master to slave as previously discussed.
> Thoughts?
Ping - did you guys reach to an agreement?
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review67113
-----------------------------------------------------------
On Jan. 8, 2015, 12:44 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Jan. 8, 2015, 12:44 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to replace the existing (still default)
> SLAVE_PING_TIMEOUT (15secs) and MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/constants.hpp c386eab
> src/master/constants.cpp 9ee17e9
> src/master/flags.hpp f5c8d2a
> src/master/master.cpp d6651e2
> src/slave/constants.hpp fd1c1ab
> src/slave/constants.cpp 2a99b11
> src/slave/slave.cpp 50b5781
> src/tests/fault_tolerance_tests.cpp 5763486
> src/tests/partition_tests.cpp fea7801
> src/tests/slave_recovery_tests.cpp cd4a398
> src/tests/slave_tests.cpp c50cbc7
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Updated unit tests to use shorter non-default values for ping timeouts.
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Adam B <ad...@mesosphere.io>.
> On Jan. 7, 2015, 2:50 p.m., Ben Mahler wrote:
> > src/master/flags.hpp, lines 361-373
> > <https://reviews.apache.org/r/29507/diff/1/?file=804645#file804645line361>
> >
> > Do you mind changing the names now that they're merely defaults?
> >
> > s/SLAVE_PING_TIMEOUT/DEFAULT_SLAVE_PING_TIMEOUT/
> > s/MAX_SLAVE_PING_TIMEOUTS/DEFAULT_MAX_SLAVE_PING_TIMEOUTS/
> >
> > Once you do this, it will become apparent there is more needed in this change. In particular, all the slave logic that depends on MASTER_PING_TIMEOUT() is broken now! Making it configurable will require some more thought.
>
> Adam B wrote:
> Renamed to make the DEFAULT nature explicit.
>
> I would argue that with this change as is, the slave side of the ping timeout isn't much more broken than it already was. As I alluded to in the MESOS-2110 JIRA description, the slave uses the timeout to force a leader-detection & re-registration, which is only loosely related to what the master is doing with the timeout (declaring the slave shutdown). Even without my changes, the Timers on master/slave could be out of sync and the two timeout events aren't guaranteed to happen in a particular order. If the master were to timeout first, and the slave did eventually detect a new leader, the slave would still be told to shutdown and its re-registration would fail. If the slave timed out first, it would try to re-register with the currently verified leader (could be the same), and the master's SlaveObserver would keep trying to ping the slave and potentially eventually shut it down. That's why I would argue that this patch is fine on its own, and a subsequent patch (same JIRA?) sh
ould clean up the slave's ping timeout behavior.
>
> `src/master/constants.hpp` has the following note:
> ```
> // NOTE: The slave uses these PING constants to determine when
> // the master has stopped sending pings. If these are made
> // configurable, then we'll need to rely on upper/lower bounds
> // to ensure that the slave is not unnecessarily triggering
> // re-registrations.
> ```
> If the master flags are configurable, there's currently no way for the slave to know what value the current leading master is using, especially if the slave is disconnected at the time. After reading through the comments in MESOS-1529 (which added the slave-side ping timeout), I'm thinking that maybe the master->slave registered (and PING?) messages could embed the current master's actual ping timeout. Then the pingTimers started from Slave::registered() and Slave::ping() could use that value (same as master's) to prevent unnecessary reregistration due to a timeout mismatch.
>
> Thoughts, suggestions?
>
> Ben Mahler wrote:
> > I would argue that with this change as is, the slave side of the ping timeout isn't much more broken than it already was.
>
> Hm.. I think you misunderstood the intent of the slave using this timeout (it's not trying to prevent the things you discussed, please clarify if you think it's causing some issues, I didn't quite understand what you're pointing out).
>
> These timers are in place to guarantee that if the master removes a slave (stops health checking it), then the slave will eventually terminate. Without this timeout, it's possible for the master -> slave removal messages to drop while the slave -> master socket remains intact. At this point, (without the timeout) the slave would remain running potentially forever (unless a message (e.g. status update) is sent to the master for the master to detect the zombied slave).
>
> The slave also uses the pings to prevent a slave from getting "stuck" in a situation where the master considers the slave disconnected, but the slave considers itself connected. This can happen via a one-way socket closure from master -> slave. Note that the health checking in the master continues while the slave is disconnected.
>
> The comments here should help:
>
> ```
> void Slave::ping(const UPID& from, bool connected)
> {
> VLOG(1) << "Received ping from " << from;
>
> if (!connected && state == RUNNING) {
> // This could happen if there is a one way partition between
> // the master and slave, causing the master to get an exited
> // event and marking the slave disconnected but the slave
> // thinking it is still connected. Force a re-registration with
> // the master to reconcile.
> LOG(INFO) << "Master marked the slave as disconnected but the slave"
> << " considers itself registered! Forcing re-registration.";
> detection.discard();
> }
>
> // If we don't get a ping from the master, trigger a
> // re-registration. This can occur when the master no
> // longer considers the slave to be registered, so it is
> // essential for the slave to attempt a re-registration
> // when this occurs.
> Clock::cancel(pingTimer);
>
> pingTimer = delay(
> MASTER_PING_TIMEOUT(),
> self(),
> &Slave::pingTimeout,
> detection);
>
> send(from, PongSlaveMessage());
> }
> ```
>
> Based on all of this, it's really important to understand the implications of the slave under and over estimating the timeout on the master side. For example, if the slave under-estimates and always triggers re-registration, that would be problematic on large clusters (40k slaves / 75 seconds = 500+ re-registrations / second in the steady state!). That's why a TODO mentioned using upper/lower bounds for this.
Thank you for reviewing/discussing. I hadn't thought much before about the one-way socket closure scenario, but it doesn't drastically change my perspective. As you mention, it's important to consider the implications of the slave misestimating the timeout, especially with a configurable timeout where a master and slave could be at different versions/configurations during a rolling upgrade. There must be one source of truth for this timeout, rather than allowing the slave to "estimate" or have its own timeout configuration. So I still propose that the master should pass `master_ping_timeout` to the slave via SlaveRe[re]gisteredMessage, so that any slave that has ever considered itself registered/RUNNING knows the master's actual value and will wait at least as long as the master, no matter if/when/how the socket closes. We could also pass the timeout in the PingSlaveMessage if we're paranoid about the master changing its ping timeout value without triggering a reregistration. If you
aren't opposed, I can put together a patch and some tests to demonstrate.
Even with the master communicating its timeout so that both sides use the same value, at the point of a two-way network split whichever side last received a Ping/Pong will start its timer later and consequently time out later due to the non-zero communication latency. In the event of a one-way socket closure where the slave can still reach the master, a successful Pong will always follow a successful Ping, so the slave will time out first.
If the master times out first, it will send a ShutdownSlaveMessage, remove the slave, and terminate the SlaveObserver, never trying to contact the slave again. The slave may continue trying to send status updates, etc. in the meantime (which might go through in a one-way disconnect), but the slave will eventually timeout, disconnect itself (so it stops sending messages), and consult the MasterDetector for a leader to reregister with. If the master cannot reach the slave, it is likely that the ZK quorum cannot reach it either, but if the slave can reach the ZK quorum, or is configured with a static master, it will indeed try to re-authenticate/register, using a backoff for registration, but not for authentication (TODO).
If the slave times out first, it will unnecessarily try to reregister/authenticate even though the master thinks it is still registered. If the master continues to send messages to the slave, they will all be dropped with a warning logged, except for Ping messages which will try to reply with a Pong. If the reregister/authenticate/pong messages from the slave cannot reach the master, the master will eventually remove the slave. If the slave is able to reregister before the master removes it (i.e. brief network connectivity issue), all will go back to normal, but the whole headache could have been avoided if the slave had timed out after the master.
To try to enforce ordering (master always times out first), we could have the master timeout after `slave_ping_timeout*max_slave_ping_timeouts` and have the slave timeout after `slave_ping_timeout*(max_slave_ping_timeouts+1)`, passing these values from the master to slave as previously discussed.
Thoughts?
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review67113
-----------------------------------------------------------
On Jan. 8, 2015, 12:44 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Jan. 8, 2015, 12:44 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2110
> https://issues.apache.org/jira/browse/MESOS-2110
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to replace the existing (still default)
> SLAVE_PING_TIMEOUT (15secs) and MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/constants.hpp c386eab
> src/master/constants.cpp 9ee17e9
> src/master/flags.hpp f5c8d2a
> src/master/master.cpp d6651e2
> src/slave/constants.hpp fd1c1ab
> src/slave/constants.cpp 2a99b11
> src/slave/slave.cpp 50b5781
> src/tests/fault_tolerance_tests.cpp 5763486
> src/tests/partition_tests.cpp fea7801
> src/tests/slave_recovery_tests.cpp cd4a398
> src/tests/slave_tests.cpp c50cbc7
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Updated unit tests to use shorter non-default values for ping timeouts.
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review67113
-----------------------------------------------------------
src/master/flags.hpp
<https://reviews.apache.org/r/29507/#comment110937>
Do you mind changing the names now that they're merely defaults?
s/SLAVE_PING_TIMEOUT/DEFAULT_SLAVE_PING_TIMEOUT/
s/MAX_SLAVE_PING_TIMEOUTS/DEFAULT_MAX_SLAVE_PING_TIMEOUTS/
Once you do this, it will become apparent there is more needed in this change. In particular, all the slave logic that depends on MASTER_PING_TIMEOUT() is broken now! Making it configurable will require some more thought.
- Ben Mahler
On Dec. 31, 2014, 9:38 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Dec. 31, 2014, 9:38 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2150
> https://issues.apache.org/jira/browse/MESOS-2150
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to replace the existing (still default)
> SLAVE_PING_TIMEOUT (15secs) and MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/flags.hpp f5c8d2a8cc7530bc8746935af9ea90af747cc111
> src/master/master.cpp d6651e299ddb73bfdc1b126c474075db6cda8acd
> src/tests/fault_tolerance_tests.cpp 5763486acb6d687b50c02c01ea00e1cfbea48421
> src/tests/mesos.cpp 3b98c69a604132be71a60fbbee4a47b51fe6956a
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_recovery_tests.cpp cd4a398ef680b5694cb6069b8e2ca4e2c05911d1
> src/tests/slave_tests.cpp c50cbc799d4793243f184f9fe628b69a020adc66
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Updated unit tests to use shorter non-default values for ping timeouts.
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Michael Park <mc...@gmail.com>.
> On Jan. 7, 2015, 8:22 p.m., Michael Park wrote:
> >
Looks good to me. Just a minor comment on consistent variable naming.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review67077
-----------------------------------------------------------
On Dec. 31, 2014, 9:38 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Dec. 31, 2014, 9:38 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2150
> https://issues.apache.org/jira/browse/MESOS-2150
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to replace the existing (still default)
> SLAVE_PING_TIMEOUT (15secs) and MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/flags.hpp f5c8d2a8cc7530bc8746935af9ea90af747cc111
> src/master/master.cpp d6651e299ddb73bfdc1b126c474075db6cda8acd
> src/tests/fault_tolerance_tests.cpp 5763486acb6d687b50c02c01ea00e1cfbea48421
> src/tests/mesos.cpp 3b98c69a604132be71a60fbbee4a47b51fe6956a
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_recovery_tests.cpp cd4a398ef680b5694cb6069b8e2ca4e2c05911d1
> src/tests/slave_tests.cpp c50cbc799d4793243f184f9fe628b69a020adc66
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Updated unit tests to use shorter non-default values for ping timeouts.
>
>
> Thanks,
>
> Adam B
>
>
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29507/#review67077
-----------------------------------------------------------
src/master/master.cpp
<https://reviews.apache.org/r/29507/#comment110906>
Maybe `s/ping_timeout/slave_ping_timeout/` and `s/max_timeouts/max_slave_ping_timeouts/`?
My guess is that you chose this naming scheme because we're already inside `SlaveObserver`. But I see `slaveInfo`, `slaveId`, as opposed to `info` and `id`, would prefer if these can be consistent one way or another :)
- Michael Park
On Dec. 31, 2014, 9:38 a.m., Adam B wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29507/
> -----------------------------------------------------------
>
> (Updated Dec. 31, 2014, 9:38 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Bugs: MESOS-2150
> https://issues.apache.org/jira/browse/MESOS-2150
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Added new --slave_ping_timeout and --max_slave_ping_timeouts flags
> to mesos-master to replace the existing (still default)
> SLAVE_PING_TIMEOUT (15secs) and MAX_SLAVE_PING_TIMEOUTS (5).
>
> These can be extended if slaves are expected/allowed to be down for
> longer than a minute or two.
>
> Beware that this affects recovery from network timeouts as well as
> actual slave node/process failover.
>
>
> Diffs
> -----
>
> src/master/flags.hpp f5c8d2a8cc7530bc8746935af9ea90af747cc111
> src/master/master.cpp d6651e299ddb73bfdc1b126c474075db6cda8acd
> src/tests/fault_tolerance_tests.cpp 5763486acb6d687b50c02c01ea00e1cfbea48421
> src/tests/mesos.cpp 3b98c69a604132be71a60fbbee4a47b51fe6956a
> src/tests/partition_tests.cpp fea78016268b007590516798eb30ff423fd0ae58
> src/tests/slave_recovery_tests.cpp cd4a398ef680b5694cb6069b8e2ca4e2c05911d1
> src/tests/slave_tests.cpp c50cbc799d4793243f184f9fe628b69a020adc66
>
> Diff: https://reviews.apache.org/r/29507/diff/
>
>
> Testing
> -------
>
> Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts.
> Updated unit tests to use shorter non-default values for ping timeouts.
>
>
> Thanks,
>
> Adam B
>
>