You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Gastón Kleiman <ga...@mesosphere.com> on 2017/01/28 12:39:49 UTC
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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
Changes
-------
Addressed Vinod's feedback.
Summary (updated)
-----------------
Added support for command health checks to the default executor.
Bugs: MESOS-6280
https://issues.apache.org/jira/browse/MESOS-6280
Repository: mesos
Description (updated)
-------
Added support for command health checks to the default executor.
Diffs (updated)
-----
src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6
src/checks/health_checker.cpp e70bd7936752613a4f92c70c4c61cd7cdf7c4ee5
src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3
src/tests/health_check_tests.cpp 710cb66eff6c4447caa22772f0cdc97cfa582c50
Diff: https://reviews.apache.org/r/55901/diff/
Testing
-------
Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS.
Thanks,
Gast�n Kleiman
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
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 command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Jan. 28, 2017, 12:39 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> 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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
>
>
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for command health checks to the default executor.
>
>
> Diffs
> -----
>
> src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6
> src/checks/health_checker.cpp e70bd7936752613a4f92c70c4c61cd7cdf7c4ee5
> src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3
> src/tests/health_check_tests.cpp 710cb66eff6c4447caa22772f0cdc97cfa582c50
>
> Diff: https://reviews.apache.org/r/55901/diff/
>
>
> Testing
> -------
>
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
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 command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 2, 2017, 5:02 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> 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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
>
>
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for command health checks to the default executor.
>
>
> Diffs
> -----
>
> src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6
> src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b
> src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3
> src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb
>
> Diff: https://reviews.apache.org/r/55901/diff/
>
>
> Testing
> -------
>
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
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 command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 3, 2017, 5:15 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> 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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
>
>
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for command health checks to the default executor.
>
>
> Diffs
> -----
>
> src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6
> src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b
> src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3
> src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb
>
> Diff: https://reviews.apache.org/r/55901/diff/
>
>
> Testing
> -------
>
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55901/#review164951
-----------------------------------------------------------
src/checks/health_checker.cpp (line 657)
<https://reviews.apache.org/r/55901/#comment236804>
I wanted to make the errors here similar to/consistent with the errors logged by the default executor:
https://github.com/apache/mesos/blob/c2388a511c775dd6f392961b06fd7738bf051dbc/src/launcher/default_executor.cpp#L618-L624
src/checks/health_checker.cpp (line 667)
<https://reviews.apache.org/r/55901/#comment236789>
It depends on what containerizer is being used. With the `MesosContainerizer`, we get the exit status as returned by `waitpid`. I added a comment in the header file.
- Gast�n Kleiman
On Feb. 8, 2017, 1:27 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> 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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
>
>
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for command health checks to the default executor.
>
>
> Diffs
> -----
>
> src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6
> src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b
> src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a
> src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81
>
> Diff: https://reviews.apache.org/r/55901/diff/
>
>
> Testing
> -------
>
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
> On Feb. 9, 2017, 3:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, lines 653-654
> > <https://reviews.apache.org/r/55901/diff/8/?file=1627619#file1627619line653>
> >
> > Why not `defer`?
Added `defer` where it corresponds.
> On Feb. 9, 2017, 3:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, line 635
> > <https://reviews.apache.org/r/55901/diff/8/?file=1627619#file1627619line635>
> >
> > Could you please specify what this function returns? Status or exit code?
This depends on what containerizer the agent is using. In the case of the Mesos containerizer, it currently returns a status code.
> On Feb. 9, 2017, 3:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, line 667
> > <https://reviews.apache.org/r/55901/diff/8/?file=1627619#file1627619line667>
> >
> > What do you get from the agent call: status or exit code directly? It's probably not that important here, but it is for checks.
This depends on what containerizer the agent is using. In the case of the Mesos containerizer, it currently returns a status code.
> On Feb. 9, 2017, 3:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, lines 565-567
> > <https://reviews.apache.org/r/55901/diff/8/?file=1627619#file1627619line565>
> >
> > This should probably be indented, no?
Fixed.
> On Feb. 9, 2017, 3:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, line 555
> > <https://reviews.apache.org/r/55901/diff/8/?file=1627619#file1627619line555>
> >
> > s/until/until after/ ?
Fixed.
> On Feb. 9, 2017, 3:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, line 615
> > <https://reviews.apache.org/r/55901/diff/8/?file=1627619#file1627619line615>
> >
> > s/this future is completed//we finish processing current timeout./
> >
> > It's not clear what "this future" means.
Fixed.
- Gast�n
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55901/#review164919
-----------------------------------------------------------
On Feb. 10, 2017, 6:40 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> 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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
>
>
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for command health checks to the default executor.
>
>
> Diffs
> -----
>
> src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
> src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00
> src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a
> src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81
>
> Diff: https://reviews.apache.org/r/55901/diff/
>
>
> Testing
> -------
>
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS, because of MESOS-7050.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
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 previous check on timeout. Please confirm such collision is not possible.
src/checks/health_checker.cpp (lines 338 - 342)
<https://reviews.apache.org/r/55901/#comment236747>
How about `checkResult = commandCheckViaAgent ? nestedCommandHealthCheck() : commandHealthCheck();` ?
src/checks/health_checker.cpp (line 555)
<https://reviews.apache.org/r/55901/#comment236753>
s/until/until after/ ?
src/checks/health_checker.cpp (lines 565 - 567)
<https://reviews.apache.org/r/55901/#comment236752>
This should probably be indented, no?
src/checks/health_checker.cpp (lines 575 - 580)
<https://reviews.apache.org/r/55901/#comment236757>
After thinking some time I think I understand why we treat any HTTP status code other than 200 as health check failure. I think it still makes sense to capture this in a comment explaining that if the agent cannot launch a nested container for _any_ reason, it is considered a health check failure. We should also mention this when we update the documentaiton.
src/checks/health_checker.cpp (line 615)
<https://reviews.apache.org/r/55901/#comment236759>
s/this future is completed//we finish processing current timeout./
It's not clear what "this future" means.
src/checks/health_checker.cpp (lines 623 - 628)
<https://reviews.apache.org/r/55901/#comment236762>
I think what you're trying to say is that it is fine to complete the future returned from this call if `waitForNestedContainer` fails, because 1) debug containers are not checkpointed and 2) `waitForNestedContainer` fails iff agent fails as well hence cleaning up the container.
Could you please rephrase the comment mentioning both your expectation (no wait retry is fine) and reasoning?
src/checks/health_checker.cpp (line 635)
<https://reviews.apache.org/r/55901/#comment236774>
Could you please specify what this function returns? Status or exit code?
src/checks/health_checker.cpp (lines 653 - 654)
<https://reviews.apache.org/r/55901/#comment236765>
Why not `defer`?
src/checks/health_checker.cpp (lines 654 - 670)
<https://reviews.apache.org/r/55901/#comment236763>
I would extract this lambda into a separate callback for readability.
src/checks/health_checker.cpp (line 657)
<https://reviews.apache.org/r/55901/#comment236771>
Maybe saying "Agent returned" instead of "Received"?
src/checks/health_checker.cpp (line 666)
<https://reviews.apache.org/r/55901/#comment236764>
please `CHECK` that `response->has_wait_nested_container()`.
src/checks/health_checker.cpp (lines 666 - 670)
<https://reviews.apache.org/r/55901/#comment236766>
```
return (
response->wait_nested_container().has_exit_status()
? response->wait_nested_container().exit_status()
: None());
```
src/checks/health_checker.cpp (line 667)
<https://reviews.apache.org/r/55901/#comment236770>
What do you get from the agent call: status or exit code directly? It's probably not that important here, but it is for checks.
- Alexander Rukletsov
On Feb. 8, 2017, 1:27 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> 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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
>
>
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for command health checks to the default executor.
>
>
> Diffs
> -----
>
> src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6
> src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b
> src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a
> src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81
>
> Diff: https://reviews.apache.org/r/55901/diff/
>
>
> Testing
> -------
>
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
> On March 22, 2017, 3:11 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp
> > Lines 601 (patched)
> > <https://reviews.apache.org/r/55901/diff/16/?file=1670849#file1670849line601>
> >
> > Please avoid shadowing. `waitFuture`?
That `Future` aint need no name!
- Gast�n
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55901/#review169691
-----------------------------------------------------------
On March 21, 2017, 2:01 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> -----------------------------------------------------------
>
> (Updated March 21, 2017, 2:01 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
>
>
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/checks/health_checker.hpp 44df544b585b8c9f1138fc69b34b064bae8cc867
> src/checks/health_checker.cpp a26e9b570ea3a0ee775d220a3b523ae7052dad23
> src/launcher/default_executor.cpp 6a885af50db6d489194cc9b780fcf1494e853694
> src/tests/health_check_tests.cpp fc1c8281ff8fbb76b7ac510ce36c98db4de4d171
>
>
> Diff: https://reviews.apache.org/r/55901/diff/16/
>
>
> Testing
> -------
>
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux and macOS.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
> On March 22, 2017, 3:11 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp
> > Lines 474 (patched)
> > <https://reviews.apache.org/r/55901/diff/16/?file=1670849#file1670849line474>
> >
> > Please escape task ids, since they are not generated by Mesos and may contain space.
> >
> > Also, you don't need to `stringify` `TaskID` here.
Fixed in https://reviews.apache.org/r/57854/
- Gast�n
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55901/#review169691
-----------------------------------------------------------
On March 21, 2017, 2:01 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> -----------------------------------------------------------
>
> (Updated March 21, 2017, 2:01 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
>
>
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/checks/health_checker.hpp 44df544b585b8c9f1138fc69b34b064bae8cc867
> src/checks/health_checker.cpp a26e9b570ea3a0ee775d220a3b523ae7052dad23
> src/launcher/default_executor.cpp 6a885af50db6d489194cc9b780fcf1494e853694
> src/tests/health_check_tests.cpp fc1c8281ff8fbb76b7ac510ce36c98db4de4d171
>
>
> Diff: https://reviews.apache.org/r/55901/diff/16/
>
>
> Testing
> -------
>
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux and macOS.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55901/#review169691
-----------------------------------------------------------
Fix it, then Ship it!
src/checks/health_checker.cpp
Lines 474 (patched)
<https://reviews.apache.org/r/55901/#comment242212>
Please escape task ids, since they are not generated by Mesos and may contain space.
Also, you don't need to `stringify` `TaskID` here.
src/checks/health_checker.cpp
Lines 601 (patched)
<https://reviews.apache.org/r/55901/#comment242213>
Please avoid shadowing. `waitFuture`?
src/tests/health_check_tests.cpp
Lines 2191 (patched)
<https://reviews.apache.org/r/55901/#comment242223>
This will very likely not work on Windows, which is fine for now, because the test is disabled on Windows.
src/tests/health_check_tests.cpp
Lines 2196-2199 (patched)
<https://reviews.apache.org/r/55901/#comment242217>
I know this is what `clang-format` suggest, but I'm not sure this formatting is not common in our codebase. Moreover, it's "jagged". I'd vote for
```
Environment::Variable* variable = healthCheck.mutable_command()->
mutable_environment()->mutable_variables()->Add();
```
- Alexander Rukletsov
On March 21, 2017, 2:01 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> -----------------------------------------------------------
>
> (Updated March 21, 2017, 2:01 p.m.)
>
>
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
>
>
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/checks/health_checker.hpp 44df544b585b8c9f1138fc69b34b064bae8cc867
> src/checks/health_checker.cpp a26e9b570ea3a0ee775d220a3b523ae7052dad23
> src/launcher/default_executor.cpp 6a885af50db6d489194cc9b780fcf1494e853694
> src/tests/health_check_tests.cpp fc1c8281ff8fbb76b7ac510ce36c98db4de4d171
>
>
> Diff: https://reviews.apache.org/r/55901/diff/16/
>
>
> Testing
> -------
>
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux and macOS.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>
Re: Review Request 55901: Added support for COMMAND health checks to
the default executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
Summary (updated)
-----------------
Added support for COMMAND health checks to the default executor.
Bugs: MESOS-6280
https://issues.apache.org/jira/browse/MESOS-6280
Repository: mesos
Description
-------
Added support for command health checks to the default executor.
Diffs
-----
src/checks/health_checker.hpp 44df544b585b8c9f1138fc69b34b064bae8cc867
src/checks/health_checker.cpp a26e9b570ea3a0ee775d220a3b523ae7052dad23
src/launcher/default_executor.cpp 6a885af50db6d489194cc9b780fcf1494e853694
src/tests/health_check_tests.cpp fc1c8281ff8fbb76b7ac510ce36c98db4de4d171
Diff: https://reviews.apache.org/r/55901/diff/19/
Testing
-------
Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux and macOS.
Thanks,
Gast�n Kleiman
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55901/
-----------------------------------------------------------
(Updated March 22, 2017, 6:19 p.m.)
Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
Changes
-------
Addresed feedback.
Bugs: MESOS-6280
https://issues.apache.org/jira/browse/MESOS-6280
Repository: mesos
Description (updated)
-------
Added support for command health checks to the default executor.
Diffs (updated)
-----
src/checks/health_checker.hpp 44df544b585b8c9f1138fc69b34b064bae8cc867
src/checks/health_checker.cpp a26e9b570ea3a0ee775d220a3b523ae7052dad23
src/launcher/default_executor.cpp 6a885af50db6d489194cc9b780fcf1494e853694
src/tests/health_check_tests.cpp fc1c8281ff8fbb76b7ac510ce36c98db4de4d171
Diff: https://reviews.apache.org/r/55901/diff/17/
Changes: https://reviews.apache.org/r/55901/diff/16-17/
Testing
-------
Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux and macOS.
Thanks,
Gast�n Kleiman
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55901/
-----------------------------------------------------------
(Updated March 21, 2017, 2:01 p.m.)
Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
Changes
-------
Fixed typos, removed single quotes from log messages and fixed flaky test in macOS.
Bugs: MESOS-6280
https://issues.apache.org/jira/browse/MESOS-6280
Repository: mesos
Description (updated)
-------
See summary.
Diffs (updated)
-----
src/checks/health_checker.hpp 44df544b585b8c9f1138fc69b34b064bae8cc867
src/checks/health_checker.cpp a26e9b570ea3a0ee775d220a3b523ae7052dad23
src/launcher/default_executor.cpp 6a885af50db6d489194cc9b780fcf1494e853694
src/tests/health_check_tests.cpp fc1c8281ff8fbb76b7ac510ce36c98db4de4d171
Diff: https://reviews.apache.org/r/55901/diff/16/
Changes: https://reviews.apache.org/r/55901/diff/15-16/
Testing (updated)
-------
Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux and macOS.
Thanks,
Gast�n Kleiman
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
Changes
-------
Rebased + updated accept type.
Bugs: MESOS-6280
https://issues.apache.org/jira/browse/MESOS-6280
Repository: mesos
Description (updated)
-------
Added support for command health checks to the default executor.
Diffs (updated)
-----
src/checks/health_checker.hpp 44df544b585b8c9f1138fc69b34b064bae8cc867
src/checks/health_checker.cpp 7efefe9455df20a038a2c9d4f67c0c0cd69c301e
src/launcher/default_executor.cpp cbd4f7ecd042e7fa603bd69774d95472df2c896d
src/tests/health_check_tests.cpp fc1c8281ff8fbb76b7ac510ce36c98db4de4d171
Diff: https://reviews.apache.org/r/55901/diff/15/
Changes: https://reviews.apache.org/r/55901/diff/14-15/
Testing
-------
Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS, because of MESOS-7050.
Thanks,
Gast�n Kleiman
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
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 505-506 (patched)
<https://reviews.apache.org/r/55901/#comment241442>
We changed the `Accept` type for streaming responses recently (though the old type is still accepted for backwards compat)
```
Accept : stringify(ContentType::RECORDIO);
Message-Accept: stringify(ContentType::ContentType::PROTOBUF)
```
- Vinod Kone
On March 15, 2017, 3:05 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> 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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
>
>
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See the summary.
>
>
> Diffs
> -----
>
> src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
> src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1
> src/launcher/default_executor.cpp 0ed436faa68e984d0be4e5186138f738bc7f1b52
> src/tests/health_check_tests.cpp fc1c8281ff8fbb76b7ac510ce36c98db4de4d171
>
>
> Diff: https://reviews.apache.org/r/55901/diff/14/
>
>
> Testing
> -------
>
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS, because of MESOS-7050.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
Changes
-------
Rebase + added a TODO.
Bugs: MESOS-6280
https://issues.apache.org/jira/browse/MESOS-6280
Repository: mesos
Description (updated)
-------
See the summary.
Diffs (updated)
-----
src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1
src/launcher/default_executor.cpp 0ed436faa68e984d0be4e5186138f738bc7f1b52
src/tests/health_check_tests.cpp fc1c8281ff8fbb76b7ac510ce36c98db4de4d171
Diff: https://reviews.apache.org/r/55901/diff/14/
Changes: https://reviews.apache.org/r/55901/diff/13-14/
Testing
-------
Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS, because of MESOS-7050.
Thanks,
Gast�n Kleiman
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
Changes
-------
Rebased + removed env inheritance.
Bugs: MESOS-6280
https://issues.apache.org/jira/browse/MESOS-6280
Repository: mesos
Description
-------
Added support for command health checks to the default executor.
Diffs (updated)
-----
src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1
src/launcher/default_executor.cpp 0ed436faa68e984d0be4e5186138f738bc7f1b52
src/tests/health_check_tests.cpp 56e90747f2c943daee675738428d8ddeeafde36d
Diff: https://reviews.apache.org/r/55901/diff/13/
Changes: https://reviews.apache.org/r/55901/diff/12-13/
Testing
-------
Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS, because of MESOS-7050.
Thanks,
Gast�n Kleiman
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
Changes
-------
Addressed feedback.
Bugs: MESOS-6280
https://issues.apache.org/jira/browse/MESOS-6280
Repository: mesos
Description
-------
Added support for command health checks to the default executor.
Diffs (updated)
-----
src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1
src/launcher/default_executor.cpp d97324b0e5eb3f145ee93aa0821205c5eefbfce0
src/tests/health_check_tests.cpp 476e04a12f885e5060a6e838b82ee599594c96f7
Diff: https://reviews.apache.org/r/55901/diff/
Testing
-------
Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS, because of MESOS-7050.
Thanks,
Gast�n Kleiman
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
Changes
-------
Addresed Vinod's feedback.
Bugs: MESOS-6280
https://issues.apache.org/jira/browse/MESOS-6280
Repository: mesos
Description
-------
Added support for command health checks to the default executor.
Diffs (updated)
-----
src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1
src/launcher/default_executor.cpp d97324b0e5eb3f145ee93aa0821205c5eefbfce0
src/tests/health_check_tests.cpp 476e04a12f885e5060a6e838b82ee599594c96f7
Diff: https://reviews.apache.org/r/55901/diff/
Testing
-------
Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS, because of MESOS-7050.
Thanks,
Gast�n Kleiman
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
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]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 10, 2017, 6:40 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> 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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
>
>
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for command health checks to the default executor.
>
>
> Diffs
> -----
>
> src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
> src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00
> src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a
> src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81
>
> Diff: https://reviews.apache.org/r/55901/diff/
>
>
> Testing
> -------
>
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS, because of MESOS-7050.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
> On Feb. 15, 2017, 9:40 p.m., Vinod Kone wrote:
> > src/checks/health_checker.cpp, line 664
> > <https://reviews.apache.org/r/55901/diff/1-10/?file=1613998#file1613998line664>
> >
> > won't we be losing the info about why wait failed?
Yes, but the health check timed out anyway, we call `WaitNestedContainer`, only to ensure that the next `LaunchNestedContainerSession` call won't fail, so I don't think that a user would care about the details of the `WaitNestedContainer` call. We could log the response `VLOG(1)` or `VLOG(2)`, to make it easier to debug potential Mesos bugs.
In our offline discussions we also realized that reusing the `ContainerID` is not always safe, so we'll have to replace this wait call with a wait + cleanupContainer.
> On Feb. 15, 2017, 9:40 p.m., Vinod Kone wrote:
> > src/checks/health_checker.cpp, line 616
> > <https://reviews.apache.org/r/55901/diff/1-10/?file=1613998#file1613998line616>
> >
> > is this deferred only because you want to access `taskId`? why not just pass taskId to the lambda directly and not-defer?
Because the compiler complains about not being able to capture a non-variable:
```
../../src/checks/health_checker.cpp:584:26: error: capture of non-variable \u2018mesos::internal::checks::HealthCheckerProcess::taskId\u2019
```
> On Feb. 15, 2017, 9:40 p.m., Vinod Kone wrote:
> > src/checks/health_checker.cpp, line 654
> > <https://reviews.apache.org/r/55901/diff/1-10/?file=1613998#file1613998line654>
> >
> > do you want to add a TODO here to not re-use the ContainerID?
I don't know if we should commit this with that TODO, or if I should wait until MESOS-7120 is resolved, and then update this patch to not re-use the ContainerID.
- Gast�n
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55901/#review165630
-----------------------------------------------------------
On Feb. 10, 2017, 6:40 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> 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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
>
>
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for command health checks to the default executor.
>
>
> Diffs
> -----
>
> src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
> src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00
> src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a
> src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81
>
> Diff: https://reviews.apache.org/r/55901/diff/
>
>
> Testing
> -------
>
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS, because of MESOS-7050.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
> On Feb. 15, 2017, 9:40 p.m., Vinod Kone wrote:
> > src/checks/health_checker.cpp
> > Lines 617 (patched)
> > <https://reviews.apache.org/r/55901/diff/1-10/?file=1613998#file1613998line654>
> >
> > do you want to add a TODO here to not re-use the ContainerID?
>
> Gast�n Kleiman wrote:
> I don't know if we should commit this with that TODO, or if I should wait until MESOS-7120 is resolved, and then update this patch to not re-use the ContainerID.
Added a TODO and addressed it in https://reviews.apache.org/r/57647/.
- Gast�n
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55901/#review165630
-----------------------------------------------------------
On March 6, 2017, 4:23 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> 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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
>
>
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for command health checks to the default executor.
>
>
> Diffs
> -----
>
> src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
> src/checks/health_checker.cpp 6c97369fd9a567ba16dd92085bf142d43f71eaf1
> src/launcher/default_executor.cpp 0ed436faa68e984d0be4e5186138f738bc7f1b52
> src/tests/health_check_tests.cpp 56e90747f2c943daee675738428d8ddeeafde36d
>
>
> Diff: https://reviews.apache.org/r/55901/diff/13/
>
>
> Testing
> -------
>
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS, because of MESOS-7050.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55901/#review165630
-----------------------------------------------------------
src/checks/health_checker.hpp (line 183)
<https://reviews.apache.org/r/55901/#comment237494>
What is it in the case of Docker containerizer? I think it is worth mentioning.
src/checks/health_checker.cpp (line 579)
<https://reviews.apache.org/r/55901/#comment237611>
is this deferred only because you want to access `taskId`? why not just pass taskId to the lambda directly and not-defer?
src/checks/health_checker.cpp (line 582)
<https://reviews.apache.org/r/55901/#comment237610>
s/of a command/of command/
src/checks/health_checker.cpp (line 608)
<https://reviews.apache.org/r/55901/#comment237616>
s/not returned/timed out/
src/checks/health_checker.cpp (line 617)
<https://reviews.apache.org/r/55901/#comment237612>
do you want to add a TODO here to not re-use the ContainerID?
src/checks/health_checker.cpp (line 619)
<https://reviews.apache.org/r/55901/#comment237625>
why does this wait has repair but not the one in `__nestedCommandHealthCheck` ?
src/checks/health_checker.cpp (line 620)
<https://reviews.apache.org/r/55901/#comment237613>
s/WaitForNestedContainer/WaitNestedContainer/
src/checks/health_checker.cpp (lines 620 - 621)
<https://reviews.apache.org/r/55901/#comment237614>
s/no matter if/irrespective of whether/
src/checks/health_checker.cpp (line 625)
<https://reviews.apache.org/r/55901/#comment237615>
s/WaitFor/Wait/
src/checks/health_checker.cpp (line 627)
<https://reviews.apache.org/r/55901/#comment237624>
won't we be losing the info about why wait failed?
src/tests/health_check_tests.cpp (line 2122)
<https://reviews.apache.org/r/55901/#comment237626>
s/is/in/
- Vinod Kone
On Feb. 10, 2017, 6:40 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> 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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
>
>
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for command health checks to the default executor.
>
>
> Diffs
> -----
>
> src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
> src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00
> src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a
> src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81
>
> Diff: https://reviews.apache.org/r/55901/diff/
>
>
> Testing
> -------
>
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS, because of MESOS-7050.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
Changes
-------
Split `waitNestedContainer` into two + use defer instead of plain lambdas.
Bugs: MESOS-6280
https://issues.apache.org/jira/browse/MESOS-6280
Repository: mesos
Description
-------
Added support for command health checks to the default executor.
Diffs (updated)
-----
src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00
src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a
src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81
Diff: https://reviews.apache.org/r/55901/diff/
Testing
-------
Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS, because of MESOS-7050.
Thanks,
Gast�n Kleiman
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
Changes
-------
Addressed feedback.
Bugs: MESOS-6280
https://issues.apache.org/jira/browse/MESOS-6280
Repository: mesos
Description
-------
Added support for command health checks to the default executor.
Diffs (updated)
-----
src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5
src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00
src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a
src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81
Diff: https://reviews.apache.org/r/55901/diff/
Testing (updated)
-------
Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS, because of MESOS-7050.
Thanks,
Gast�n Kleiman
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Gastón Kleiman <ga...@gmail.com>.
-----------------------------------------------------------
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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
Changes
-------
Fixed broken rebase.
Bugs: MESOS-6280
https://issues.apache.org/jira/browse/MESOS-6280
Repository: mesos
Description
-------
Added support for command health checks to the default executor.
Diffs (updated)
-----
src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6
src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b
src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a
src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81
Diff: https://reviews.apache.org/r/55901/diff/
Testing
-------
Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS.
Thanks,
Gast�n Kleiman
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Gastón Kleiman <ga...@mesosphere.com>.
-----------------------------------------------------------
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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
Changes
-------
Added a comment in the new test.
Bugs: MESOS-6280
https://issues.apache.org/jira/browse/MESOS-6280
Repository: mesos
Description
-------
Added support for command health checks to the default executor.
Diffs (updated)
-----
src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6
src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b
src/launcher/default_executor.cpp 8e942c6faf6e2dbc70a6fff1629343c9a3ce8668
src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81
Diff: https://reviews.apache.org/r/55901/diff/
Testing
-------
Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS.
Thanks,
Gast�n Kleiman
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Gastón Kleiman <ga...@mesosphere.com>.
-----------------------------------------------------------
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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
Changes
-------
Addressed Alex's comments.
Bugs: MESOS-6280
https://issues.apache.org/jira/browse/MESOS-6280
Repository: mesos
Description
-------
Added support for command health checks to the default executor.
Diffs (updated)
-----
src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6
src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b
src/launcher/default_executor.cpp 8e942c6faf6e2dbc70a6fff1629343c9a3ce8668
src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81
Diff: https://reviews.apache.org/r/55901/diff/
Testing
-------
Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS.
Thanks,
Gast�n Kleiman
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Gastón Kleiman <ga...@mesosphere.com>.
> On Feb. 7, 2017, 4:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, lines 618-624
> > <https://reviews.apache.org/r/55901/diff/5/?file=1623742#file1623742line618>
> >
> > 1) You have no interest in both `future` and `status`. Omit the names for clarity, please.
> >
> > 2) Why do we need to call `waitForNestedContainer` here instead of simply returning `failure`?
Added a comment clarifying this.
> On Feb. 7, 2017, 4:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, line 653
> > <https://reviews.apache.org/r/55901/diff/5/?file=1623742#file1623742line653>
> >
> > Don't you need to check that `wait_nested_container().exit_status()` exists? If `exit_status` is not set in the proto, don't you have to check this explicitly and return `None`?
Very good catch, thanks!
> On Feb. 7, 2017, 4:20 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, lines 585-588
> > <https://reviews.apache.org/r/55901/diff/5/?file=1623742#file1623742line585>
> >
> > Mention `taskId`?
Updated all the comments I could find.
- Gast�n
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55901/#review164501
-----------------------------------------------------------
On Feb. 7, 2017, 11:37 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> 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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
>
>
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for command health checks to the default executor.
>
>
> Diffs
> -----
>
> src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6
> src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b
> src/launcher/default_executor.cpp 8e942c6faf6e2dbc70a6fff1629343c9a3ce8668
> src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81
>
> Diff: https://reviews.apache.org/r/55901/diff/
>
>
> Testing
> -------
>
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55901/#review164501
-----------------------------------------------------------
src/checks/health_checker.cpp (line 512)
<https://reviews.apache.org/r/55901/#comment236268>
You can use `->` for options instead of `.get()`.
src/checks/health_checker.cpp (lines 585 - 588)
<https://reviews.apache.org/r/55901/#comment236282>
Mention `taskId`?
src/checks/health_checker.cpp (line 594)
<https://reviews.apache.org/r/55901/#comment236283>
How about "WAIT_NESTED_CONTAINER API call..."
src/checks/health_checker.cpp (lines 618 - 624)
<https://reviews.apache.org/r/55901/#comment236279>
1) You have no interest in both `future` and `status`. Omit the names for clarity, please.
2) Why do we need to call `waitForNestedContainer` here instead of simply returning `failure`?
src/checks/health_checker.cpp (lines 640 - 641)
<https://reviews.apache.org/r/55901/#comment236278>
Is this formatting correct? Is it what clang-format suggests?
src/checks/health_checker.cpp (line 646)
<https://reviews.apache.org/r/55901/#comment236280>
"... while waiting..."?
src/checks/health_checker.cpp (line 653)
<https://reviews.apache.org/r/55901/#comment236281>
Don't you need to check that `wait_nested_container().exit_status()` exists? If `exit_status` is not set in the proto, don't you have to check this explicitly and return `None`?
- Alexander Rukletsov
On Feb. 3, 2017, 7:52 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> 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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
>
>
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for command health checks to the default executor.
>
>
> Diffs
> -----
>
> src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6
> src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b
> src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3
> src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb
>
> Diff: https://reviews.apache.org/r/55901/diff/
>
>
> Testing
> -------
>
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Gastón Kleiman <ga...@mesosphere.com>.
-----------------------------------------------------------
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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
Changes
-------
Split into two patches.
Bugs: MESOS-6280
https://issues.apache.org/jira/browse/MESOS-6280
Repository: mesos
Description
-------
Added support for command health checks to the default executor.
Diffs (updated)
-----
src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6
src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b
src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3
src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb
Diff: https://reviews.apache.org/r/55901/diff/
Testing
-------
Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS.
Thanks,
Gast�n Kleiman
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Gastón Kleiman <ga...@mesosphere.com>.
-----------------------------------------------------------
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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
Changes
-------
Addressed feedback.
Bugs: MESOS-6280
https://issues.apache.org/jira/browse/MESOS-6280
Repository: mesos
Description
-------
Added support for command health checks to the default executor.
Diffs (updated)
-----
src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6
src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b
src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3
src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb
Diff: https://reviews.apache.org/r/55901/diff/
Testing
-------
Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS.
Thanks,
Gast�n Kleiman
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/tests/health_check_tests.cpp, lines 2154-2155
> > <https://reviews.apache.org/r/55901/diff/3/?file=1622284#file1622284line2154>
> >
> > Formatting.
> >
> > Also it is probably a good idea to explicitly say you don't care about further updates.
>
> Gast�n Kleiman wrote:
> Fixed the formatting. We don't have such a comment in most of the other tests in this file.
Oh, under say I mean `.WillRepeatedly(Return());`
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55901/#review164117
-----------------------------------------------------------
On Feb. 3, 2017, 7:52 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> 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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
>
>
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for command health checks to the default executor.
>
>
> Diffs
> -----
>
> src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6
> src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b
> src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3
> src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb
>
> Diff: https://reviews.apache.org/r/55901/diff/
>
>
> Testing
> -------
>
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Gastón Kleiman <ga...@mesosphere.com>.
> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, line 130
> > <https://reviews.apache.org/r/55901/diff/3/?file=1622282#file1622282line130>
> >
> > s/createRequest/createV1AgentAPIRequest?
> > remove `inline`
> >
> > This looks like a utility function not really related to the health checker library. Maybe put it into "src/slave/api_utils.{hpp|cpp}"?
>
> Gast�n Kleiman wrote:
> I renamed the function. Vinod suggested to make it `inline` what's wrong with that?
>
> This function as-is is specific to how we perform requests in this file (`keep_alive = false`, protobuf serialization). Since it is used only in two places, I'm tempted to removed it and put the code inline in the corresponding methods.
I removed these functions.
- Gast�n
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55901/#review164117
-----------------------------------------------------------
On Feb. 7, 2017, 11:37 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> 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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
>
>
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for command health checks to the default executor.
>
>
> Diffs
> -----
>
> src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6
> src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b
> src/launcher/default_executor.cpp 8e942c6faf6e2dbc70a6fff1629343c9a3ce8668
> src/tests/health_check_tests.cpp 7b6a803a28b2e4f6c27e9a0c4f668350ec2d5a81
>
> Diff: https://reviews.apache.org/r/55901/diff/
>
>
> Testing
> -------
>
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/tests/health_check_tests.cpp, lines 2118-2119
> > <https://reviews.apache.org/r/55901/diff/3/?file=1622284#file1622284line2118>
> >
> > Why do we need to expilictly create `containerizer`?
>
> Gast�n Kleiman wrote:
> Because the implicit `containerizer` is started in local mode. `LaunchNestedContainerSession` (used by cmd health checks) tries to start a IO switchboard, which doesn't work in local mode yet.
Please capture this in a comment.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55901/#review164117
-----------------------------------------------------------
On Feb. 3, 2017, 7:52 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> 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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
>
>
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for command health checks to the default executor.
>
>
> Diffs
> -----
>
> src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6
> src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b
> src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3
> src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb
>
> Diff: https://reviews.apache.org/r/55901/diff/
>
>
> Testing
> -------
>
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Gastón Kleiman <ga...@mesosphere.com>.
> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/tests/health_check_tests.cpp, lines 2118-2119
> > <https://reviews.apache.org/r/55901/diff/3/?file=1622284#file1622284line2118>
> >
> > Why do we need to expilictly create `containerizer`?
Because the implicit `containerizer` is started in local mode. `LaunchNestedContainerSession` (used by cmd health checks) tries to start a IO switchboard, which doesn't work in local mode yet.
> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.hpp, line 207
> > <https://reviews.apache.org/r/55901/diff/3/?file=1622281#file1622281line207>
> >
> > How about calling it `commandCheckViaAgent`?
Good idea, done.
> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/checks/health_checker.cpp, line 130
> > <https://reviews.apache.org/r/55901/diff/3/?file=1622282#file1622282line130>
> >
> > s/createRequest/createV1AgentAPIRequest?
> > remove `inline`
> >
> > This looks like a utility function not really related to the health checker library. Maybe put it into "src/slave/api_utils.{hpp|cpp}"?
I renamed the function. Vinod suggested to make it `inline` what's wrong with that?
This function as-is is specific to how we perform requests in this file (`keep_alive = false`, protobuf serialization). Since it is used only in two places, I'm tempted to removed it and put the code inline in the corresponding methods.
> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/tests/health_check_tests.cpp, line 2107
> > <https://reviews.apache.org/r/55901/diff/3/?file=1622284#file1622284line2107>
> >
> > Why not spelling out `cmd`?
Renamed.
> On Feb. 3, 2017, 1:34 p.m., Alexander Rukletsov wrote:
> > src/tests/health_check_tests.cpp, lines 2154-2155
> > <https://reviews.apache.org/r/55901/diff/3/?file=1622284#file1622284line2154>
> >
> > Formatting.
> >
> > Also it is probably a good idea to explicitly say you don't care about further updates.
Fixed the formatting. We don't have such a comment in most of the other tests in this file.
- Gast�n
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55901/#review164117
-----------------------------------------------------------
On Feb. 3, 2017, 5:15 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> 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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
>
>
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for command health checks to the default executor.
>
>
> Diffs
> -----
>
> src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6
> src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b
> src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3
> src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb
>
> Diff: https://reviews.apache.org/r/55901/diff/
>
>
> Testing
> -------
>
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55901/#review164117
-----------------------------------------------------------
src/checks/health_checker.hpp (line 94)
<https://reviews.apache.org/r/55901/#comment235698>
Could you please add a todo here saying this will go away and link a jira ticket?
src/checks/health_checker.hpp (line 207)
<https://reviews.apache.org/r/55901/#comment235702>
How about calling it `commandCheckViaAgent`?
src/checks/health_checker.cpp (line 130)
<https://reviews.apache.org/r/55901/#comment235700>
s/createRequest/createV1AgentAPIRequest?
remove `inline`
This looks like a utility function not really related to the health checker library. Maybe put it into "src/slave/api_utils.{hpp|cpp}"?
src/checks/health_checker.cpp (lines 146 - 170)
<https://reviews.apache.org/r/55901/#comment235701>
What is the value of using these helpers? I'd say having `connection.send(createRequest(url, call), false);` is clearer and self explanatory rather than `post(connection, url, call)`. And you don't need any comments then : )
src/checks/health_checker.cpp (line 224)
<https://reviews.apache.org/r/55901/#comment235699>
You can use `{}` instead.
src/checks/health_checker.cpp (lines 434 - 436)
<https://reviews.apache.org/r/55901/#comment235703>
Use `CHECK_SOME` instead.
src/checks/health_checker.cpp (lines 456 - 482)
<https://reviews.apache.org/r/55901/#comment235697>
Could you please link a JIRA here?
src/checks/health_checker.cpp (line 593)
<https://reviews.apache.org/r/55901/#comment235704>
Please keep the order of the definitions in sync with the order of declarations.
src/checks/health_checker.cpp (line 746)
<https://reviews.apache.org/r/55901/#comment235705>
Let's pull this (together with another below) into a separate patch.
src/tests/health_check_tests.cpp (line 2107)
<https://reviews.apache.org/r/55901/#comment235706>
Why not spelling out `cmd`?
src/tests/health_check_tests.cpp (lines 2118 - 2119)
<https://reviews.apache.org/r/55901/#comment235707>
Why do we need to expilictly create `containerizer`?
src/tests/health_check_tests.cpp (lines 2154 - 2155)
<https://reviews.apache.org/r/55901/#comment235708>
Formatting.
Also it is probably a good idea to explicitly say you don't care about further updates.
src/tests/health_check_tests.cpp (lines 2157 - 2161)
<https://reviews.apache.org/r/55901/#comment235709>
1. I think you can also check that task's env vars are available in the health check -> pass exit status in the task's command.
2. Let's avoid using obscure `populateTasks()` here. We'll get rid of it altogether in the nearest future.
src/tests/health_check_tests.cpp (lines 2185 - 2186)
<https://reviews.apache.org/r/55901/#comment235710>
check that `healthy` is set please.
- Alexander Rukletsov
On Feb. 2, 2017, 5:02 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> 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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
>
>
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added support for command health checks to the default executor.
>
>
> Diffs
> -----
>
> src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6
> src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b
> src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3
> src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb
>
> Diff: https://reviews.apache.org/r/55901/diff/
>
>
> Testing
> -------
>
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>
Re: Review Request 55901: Added support for command health checks to
the default executor.
Posted by Gastón Kleiman <ga...@mesosphere.com>.
-----------------------------------------------------------
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 Rukletsov, Anand Mazumdar, haosdent huang, and Vinod Kone.
Changes
-------
Addressed feedback + rebase.
Bugs: MESOS-6280
https://issues.apache.org/jira/browse/MESOS-6280
Repository: mesos
Description
-------
Added support for command health checks to the default executor.
Diffs (updated)
-----
src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6
src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b
src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3
src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb
Diff: https://reviews.apache.org/r/55901/diff/
Testing
-------
Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It passes on Linux, but not on macOS.
Thanks,
Gast�n Kleiman