Re: Review Request 55901: Added support for COMMAND health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/ --- (Updated March 23, 2017, 11:39 p.m.) Review request for mesos, Alexander

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/ --- (Updated March 16, 2017, 2:31 p.m.) Review request for mesos, Alexander

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/#review169120 --- Fix it, then Ship it! src/checks/health_checker.cpp Lines

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/ --- (Updated March 15, 2017, 3:05 p.m.) Review request for mesos, Alexander

Re: Review Request 55901: Added support for command health checks to the default executor.

> On Feb. 15, 2017, 9:40 p.m., Vinod Kone wrote: > > src/checks/health_checker.cpp > > Lines 617 (patched) > > > > > > do you want to add a TODO here to not re-use the ContainerID? > > Gastón Kleiman wrote: >

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/ --- (Updated March 6, 2017, 4:23 p.m.) Review request for mesos, Alexander

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/ --- (Updated Feb. 17, 2017, 11:59 a.m.) Review request for mesos, Alexander

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/ --- (Updated Feb. 16, 2017, 2:59 p.m.) Review request for mesos, Alexander

Re: Review Request 55901: Added support for command health checks to the default executor.

> On Feb. 15, 2017, 9:40 p.m., Vinod Kone wrote: > > src/checks/health_checker.cpp, line 664 > > > > > > won't we be losing the info about why wait failed? Yes, but the health check timed out anyway, we call

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/#review165630 --- src/checks/health_checker.hpp (line 183)

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/#review165230 --- Patch looks great! Reviews applied: [55900, 56288, 55901]

Re: Review Request 55901: Added support for command health checks to the default executor.

> On Feb. 9, 2017, 3:20 p.m., Alexander Rukletsov wrote: > > src/checks/health_checker.cpp, lines 653-654 > > > > > > Why not `defer`? Added `defer` where it corresponds. > On Feb. 9, 2017, 3:20 p.m., Alexander

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/ --- (Updated Feb. 10, 2017, 6:40 p.m.) Review request for mesos, Alexander

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/ --- (Updated Feb. 10, 2017, 11:25 a.m.) Review request for mesos, Alexander

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/#review164951 --- src/checks/health_checker.cpp (line 657)

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/#review164919 --- Fix it, then Ship it! LGTM modulo possible collision with the

Re: Review Request 55901: Added support for command health checks to the default executor.

> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote: > > src/checks/health_checker.hpp, line 137 > > > > > > we don't typically do `const` for POD types. > > > > s/_agentSpawnsCommandContainer/ > > Gastón

Re: Review Request 55901: Added support for command health checks to the default executor.

> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote: > > src/checks/health_checker.cpp, lines 426-429 > > > > > > why a `.repair()` here? > > Gastón Kleiman wrote: > To fail the future/check with a nice descriptive

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/ --- (Updated Feb. 8, 2017, 1:27 p.m.) Review request for mesos, Alexander

Re: Review Request 55901: Added support for command health checks to the default executor.

> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote: > > src/checks/health_checker.hpp, line 137 > > > > > > we don't typically do `const` for POD types. > > > > s/_agentSpawnsCommandContainer/ > > Gastón

Re: Review Request 55901: Added support for command health checks to the default executor.

> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote: > > src/checks/health_checker.cpp, lines 426-429 > > > > > > why a `.repair()` here? > > Gastón Kleiman wrote: > To fail the future/check with a nice descriptive

Re: Review Request 55901: Added support for command health checks to the default executor.

> On Feb. 7, 2017, 4:20 p.m., Alexander Rukletsov wrote: > > src/checks/health_checker.cpp, lines 618-624 > > > > > > 1) You have no interest in both `future` and `status`. Omit the names > > for clarity, please. >

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/ --- (Updated Feb. 7, 2017, 11:37 p.m.) Review request for mesos, Alexander

Re: Review Request 55901: Added support for command health checks to the default executor.

> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote: > > src/checks/health_checker.cpp, line 130 > > > > > > s/createRequest/createV1AgentAPIRequest? > > remove `inline` > > > > This looks like a

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/ --- (Updated Feb. 7, 2017, 11:32 p.m.) Review request for mesos, Alexander

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/#review164501 --- src/checks/health_checker.cpp (line 512)

Re: Review Request 55901: Added support for command health checks to the default executor.

> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote: > > src/checks/health_checker.cpp, lines 426-429 > > > > > > why a `.repair()` here? > > Gastón Kleiman wrote: > To fail the future/check with a nice descriptive

Re: Review Request 55901: Added support for command health checks to the default executor.

> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote: > > src/checks/health_checker.hpp, line 137 > > > > > > we don't typically do `const` for POD types. > > > > s/_agentSpawnsCommandContainer/ > > Gastón

Re: Review Request 55901: Added support for command health checks to the default executor.

> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote: > > src/checks/health_checker.hpp, line 137 > > > > > > we don't typically do `const` for POD types. > > > > s/_agentSpawnsCommandContainer/ > > Gastón

Re: Review Request 55901: Added support for command health checks to the default executor.

> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote: > > src/tests/health_check_tests.cpp, lines 2154-2155 > > > > > > Formatting. > > > > Also it is probably a good idea to explicitly say you don't

Re: Review Request 55901: Added support for command health checks to the default executor.

> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote: > > src/tests/health_check_tests.cpp, lines 2118-2119 > > > > > > Why do we need to expilictly create `containerizer`? > > Gastón Kleiman wrote: >

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/ --- (Updated Feb. 3, 2017, 7:52 p.m.) Review request for mesos, Alexander

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/#review164138 --- Patch looks great! Reviews applied: [55900, 55901] Passed

Re: Review Request 55901: Added support for command health checks to the default executor.

> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote: > > src/tests/health_check_tests.cpp, lines 2118-2119 > > > > > > Why do we need to expilictly create `containerizer`? Because the implicit `containerizer`

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/ --- (Updated Feb. 3, 2017, 5:15 p.m.) Review request for mesos, Alexander

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/#review164117 --- src/checks/health_checker.hpp (line 94)

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/#review164003 --- Patch looks great! Reviews applied: [55900, 55901] Passed

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/ --- (Updated Feb. 2, 2017, 5:02 p.m.) Review request for mesos, Alexander

Re: Review Request 55901: Added support for command health checks to the default executor.

> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote: > > src/checks/health_checker.cpp, lines 426-429 > > > > > > why a `.repair()` here? > > Gastón Kleiman wrote: > To fail the future/check with a nice descriptive

Re: Review Request 55901: Added support for command health checks to the default executor.

> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote: > > src/checks/health_checker.cpp, line 480 > > > > > > launch nested container session returns a streaming response, how come > > you are calling `post()` helper

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/#review163402 --- Patch looks great! Reviews applied: [55900, 55901] Passed

Re: Review Request 55901: Added support for command health checks to the default executor.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55901/ --- (Updated Jan. 28, 2017, 12:39 p.m.) Review request for mesos, Alexander

Re: Review Request 55901: Added support for command health checks to the default executor.

> On Jan. 28, 2017, 1:17 a.m., Vinod Kone wrote: > > src/checks/health_checker.cpp, line 480 > > > > > > launch nested container session returns a streaming response, how come > > you are calling `post()` helper