Re: Review Request 29507: Added Configurable Slave Ping Timeouts

2015-06-29 Thread Adam B
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

Re: Review Request 29507: Added Configurable Slave Ping Timeouts

2015-06-26 Thread Adam B
--- 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

Re: Review Request 29507: Added Configurable Slave Ping Timeouts

2015-06-26 Thread Adam B
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

Re: Review Request 29507: Added Configurable Slave Ping Timeouts

2015-06-26 Thread Ben Mahler
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

Re: Review Request 29507: Added Configurable Slave Ping Timeouts

2015-06-26 Thread Adam B
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

Re: Review Request 29507: Added Configurable Slave Ping Timeouts

2015-06-26 Thread Mesos ReviewBot
--- 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. -

Re: Review Request 29507: Added Configurable Slave Ping Timeouts

2015-06-24 Thread Ben Mahler
--- 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

Re: Review Request 29507: Added Configurable Slave Ping Timeouts

2015-06-24 Thread Niklas Nielsen
--- 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.,

Re: Review Request 29507: Added Configurable Slave Ping Timeouts

2015-06-22 Thread Mesos ReviewBot
--- 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. -

Re: Review Request 29507: Added Configurable Slave Ping Timeouts

2015-06-22 Thread Adam B
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

Re: Review Request 29507: Added Configurable Slave Ping Timeouts

2015-06-22 Thread Adam B
--- 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

Re: Review Request 29507: Added Configurable Slave Ping Timeouts

2015-06-01 Thread Niklas Nielsen
--- 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

Re: Review Request 29507: Added Configurable Slave Ping Timeouts

2015-05-28 Thread Adam B
--- 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

Re: Review Request 29507: Added Configurable Slave Ping Timeouts

2015-05-28 Thread Adam B
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.

Re: Review Request 29507: Added Configurable Slave Ping Timeouts

2015-05-28 Thread Mesos ReviewBot
--- 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. -

Re: Review Request 29507: Added Configurable Slave Ping Timeouts

2015-05-21 Thread Niklas Nielsen
--- 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,

Re: Review Request 29507: Added Configurable Slave Ping Timeouts

2015-05-14 Thread Adam B
--- 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

Re: Review Request 29507: Added Configurable Slave Ping Timeouts

2015-05-14 Thread Adam B
--- 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