----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57646/#review169714 -----------------------------------------------------------
Fix it, then Ship it! src/checks/health_checker.cpp Lines 380-381 (patched) <https://reviews.apache.org/r/57646/#comment242243> Please `<<` and escape task id. src/checks/health_checker.cpp Lines 489-490 (original), 498-499 (patched) <https://reviews.apache.org/r/57646/#comment242244> In such case, we discard the future... src/checks/health_checker.cpp Line 491 (original), 500 (patched) <https://reviews.apache.org/r/57646/#comment242245> "has happened" probably? src/checks/health_checker.cpp Lines 503-509 (patched) <https://reviews.apache.org/r/57646/#comment242255> I'm not sure we should keep it. Wihtout knowing all the offline discussions we had, this comment can be misleading, e.g., which future exactly do you mean or why would we say `associate` if some failures should be mapped to discards. My understanding is that you use an extra promise here becase there are two different events, which does not always conincide with their states, i.e., _some_ connection failures should map to health result discards. I'd rather say what the promise you introduce represents, e.g., "Represents the result of a health check". src/checks/health_checker.cpp Lines 513 (patched) <https://reviews.apache.org/r/57646/#comment242270> Do you really need to capture `this`? src/checks/health_checker.cpp Lines 514-515 (patched) <https://reviews.apache.org/r/57646/#comment242258> Please carry space onto the next line. src/checks/health_checker.cpp Lines 586-587 (patched) <https://reviews.apache.org/r/57646/#comment242260> Please carry space onto the next line; escape task id src/checks/health_checker.cpp Lines 612 (patched) <https://reviews.apache.org/r/57646/#comment242262> Not sure you need this line. src/checks/health_checker.cpp Lines 630-634 (patched) <https://reviews.apache.org/r/57646/#comment242266> And what if a health check segfaults? Then we'll end up dropping health checks instead of reporting failures. Also, I'm not sure this will work on windows. Please add a TODO or make sure it works there : ) src/checks/health_checker.cpp Lines 681-684 (patched) <https://reviews.apache.org/r/57646/#comment242273> This comment includes non-local information which tends to become stale. Instead, how about we simply return `Failure(failure)` here and remove the comment? - Alexander Rukletsov On March 21, 2017, 2:07 p.m., Gastón Kleiman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57646/ > ----------------------------------------------------------- > > (Updated March 21, 2017, 2:07 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 > > > Diff: https://reviews.apache.org/r/57646/diff/2/ > > > Testing > ------- > > `make check` in Linux > > > Thanks, > > Gastón Kleiman > >
