----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36816/#review145451 -----------------------------------------------------------
src/health-check/health_checker.hpp (line 275) <https://reviews.apache.org/r/36816/#comment211695> It's good to have a default. Let's create a class constant `DEFAULT_HTTP_SCHEME`. src/health-check/health_checker.hpp (lines 276 - 278) <https://reviews.apache.org/r/36816/#comment211686> This belongs into the `validate()` function. src/health-check/health_checker.hpp (line 280) <https://reviews.apache.org/r/36816/#comment211693> Do you think we should add "/" if the user has not specified anything? src/health-check/health_checker.hpp (line 282) <https://reviews.apache.org/r/36816/#comment211696> Again, it's great we can deduce the domain for the user, but let's put into the class constant, e.g. `DEFAULT_DOMAIN`. src/health-check/health_checker.hpp (line 304) <https://reviews.apache.org/r/36816/#comment211692> We can remove "with reason", what do you think? src/health-check/health_checker.hpp (lines 320 - 333) <https://reviews.apache.org/r/36816/#comment211689> This is used once, maybe it makes sense to replace it with a lambda then? The function is short enough, it's declaration is much longer than the function itself. src/health-check/health_checker.hpp (line 330) <https://reviews.apache.org/r/36816/#comment211698> Why do you need to discard original future here? src/health-check/health_checker.hpp (line 332) <https://reviews.apache.org/r/36816/#comment211697> It's unclear, what exactly is pending. How about: "CURL has not returned after " + stringify(timeout) + "; aborting" src/health-check/health_checker.hpp (line 356) <https://reviews.apache.org/r/36816/#comment211704> We should be consistent: either wrap `curl` in '' everywhere or nowhere. src/health-check/health_checker.hpp (lines 376 - 377) <https://reviews.apache.org/r/36816/#comment211705> Let's swap these conditions : ) src/health-check/health_checker.hpp (lines 392 - 393) <https://reviews.apache.org/r/36816/#comment211699> I would omit "with reason", ':' is enough. Let's make sure we test these paths. src/health-check/health_checker.cpp (lines 57 - 59) <https://reviews.apache.org/r/36816/#comment211684> Let's introduce a separate `validate` function. See my comments in the previous review. - Alexander Rukletsov On Aug. 7, 2016, 6:21 p.m., haosdent huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36816/ > ----------------------------------------------------------- > > (Updated Aug. 7, 2016, 6:21 p.m.) > > > Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gilbert Song, > Jie Yu, and Timothy Chen. > > > Bugs: MESOS-2533 > https://issues.apache.org/jira/browse/MESOS-2533 > > > Repository: mesos > > > Description > ------- > > Supported HTTP/HTTPS in health check. > > > Diffs > ----- > > src/health-check/health_checker.hpp > b28a9cf3c6c9217c0b2453ac6e34d88e1f648d61 > src/health-check/health_checker.cpp > 585a0b565d948cfa292bad818a710501a4ce0daf > > Diff: https://reviews.apache.org/r/36816/diff/ > > > Testing > ------- > > * Add unit test cases: HealthCheckTest.HealthyTaskViaHttp and > HealthCheckTest.HealthyTaskNotMatchHttpStatuses > make check > > > Thanks, > > haosdent huang > >
