----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59873/#review177306 -----------------------------------------------------------
src/checks/health_checker.hpp Line 39 (original), 32-33 (patched) <https://reviews.apache.org/r/59873/#comment250848> Insert a blank line please. src/checks/health_checker.cpp Lines 255-256 (patched) <https://reviews.apache.org/r/59873/#comment250849> I would still do it like we do it in `CheckerProcess`. src/checks/health_checker.cpp Line 199 (original), 263-264 (patched) <https://reviews.apache.org/r/59873/#comment250852> I think we can do it `CheckerProcess` src/checks/health_checker.cpp Lines 222-271 (original), 310-326 (patched) <https://reviews.apache.org/r/59873/#comment250836> How about this: ``` void HealthChecker::processCheckResult(const Try<CheckStatusInfo>& result) { if (result.isError()) { // The error is with the underlying check. LOG(WARNING) << name << " for task '" << taskId << "'" << " failed: " << result.error(); failure(); return; } Try<Nothing> healthCheckResult = interpretCheckStatusInfo(result.get()); if (healthCheckResult.isError()) { // The underlying check succeeded, but its result is interpreted as failure. LOG(WARNING) << name << " for task '" << taskId << "'" << " failed: " << healthCheckResult.error(); failure(); return; } success(); } ``` ? Note the changed signature of `failure()`. src/checks/health_checker.cpp Lines 249-251 (original), 313-315 (patched) <https://reviews.apache.org/r/59873/#comment250827> We can profit from using `name` here. src/checks/health_checker.cpp Lines 249-251 (original), 313-315 (patched) <https://reviews.apache.org/r/59873/#comment250834> This contains a lot of duplicated text with `failure()`. src/checks/health_checker.cpp Lines 290-292 (original), 335-338 (patched) <https://reviews.apache.org/r/59873/#comment250828> We can profit from using `name` here as well. src/checks/health_checker.cpp Lines 298-300 (original), 343-345 (patched) <https://reviews.apache.org/r/59873/#comment250835> Here we should probably print `consecutiveFailures`, because a health check may fail consecutively for various reasons. src/checks/health_checker.cpp Lines 388-392 (patched) <https://reviews.apache.org/r/59873/#comment250853> We don't validate other `Duration`s this way neither here not for `CheckInfo`. Can we do it consistently or update all occurences? Also, pleasa not in this patch : ) - Alexander Rukletsov On June 7, 2017, 11:56 p.m., Gastón Kleiman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59873/ > ----------------------------------------------------------- > > (Updated June 7, 2017, 11:56 p.m.) > > > Review request for mesos, Alexander Rukletsov and Vinod Kone. > > > Bugs: MESOS-7092 > https://issues.apache.org/jira/browse/MESOS-7092 > > > Repository: mesos > > > Description > ------- > > `HealthChecker` now wraps a `CheckerProcess` and gets check status > updates via a callback. > > > Diffs > ----- > > src/checks/checker.cpp dcc3164f3b623de73bacf024ede95b40c48f7192 > src/checks/checker_process.hpp PRE-CREATION > src/checks/checker_process.cpp PRE-CREATION > src/checks/health_checker.hpp 25bf7e99bf5982fd7a6ff5012c231ff89fb7cb39 > src/checks/health_checker.cpp 9d8c8471caa05af3908d34315dbfed08a343c0f8 > > > Diff: https://reviews.apache.org/r/59873/diff/2/ > > > Testing > ------- > > Tests still pass on GNU/Linux. > > > Thanks, > > Gastón Kleiman > >
