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