----------------------------------------------------------- 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 > >
