----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36816/#review93254 -----------------------------------------------------------
First pass over non-test code. Looks good. I'll give it a closer look after the next revision. include/mesos/mesos.proto (lines 199 - 200) <https://reviews.apache.org/r/36816/#comment147613> Update comment include/mesos/mesos.proto (lines 212 - 213) <https://reviews.apache.org/r/36816/#comment147614> The second sentence isn't true in your current implementation. Add a check for an empty statuses list and return success. Please add a test for this as well. include/mesos/mesos.proto (line 223) <https://reviews.apache.org/r/36816/#comment147609> Please update this comment. src/health-check/main.cpp (line 233) <https://reviews.apache.org/r/36816/#comment147608> Maybe we should add `http.protocol()` in case the user wants https? Or `http.ssl` like BenH suggested. Would we ever want anything besides http/https? We can always add that in a later patch, so feel free to ignore for now. src/health-check/main.cpp (line 241) <https://reviews.apache.org/r/36816/#comment147610> "failed with error" is redundant. Please remove and just add query.failure() src/health-check/main.cpp (line 254) <https://reviews.apache.org/r/36816/#comment147611> s/auto/uint32/ Re: `auto`: "The main goal is to increase code readability. This is safely the case if the exact same type omitted on the left is already fully stated on the right." - http://mesos.apache.org/documentation/latest/mesos-c++-style-guide/ src/health-check/main.cpp (line 260) <https://reviews.apache.org/r/36816/#comment147612> s/Health http check return/HTTP health check returned/ - Adam B On July 25, 2015, 11:57 a.m., haosdent huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36816/ > ----------------------------------------------------------- > > (Updated July 25, 2015, 11:57 a.m.) > > > Review request for mesos and Adam B. > > > Bugs: MESOS-2533 > https://issues.apache.org/jira/browse/MESOS-2533 > > > Repository: mesos > > > Description > ------- > > Support HTTP checks in Mesos health check program > > > Diffs > ----- > > include/mesos/mesos.proto a6748d1cd82238f005c6a49c70d22d095462f1ba > src/health-check/main.cpp 97b25716335ec5719c1100bd73d06b7fc98036c9 > src/tests/health_check_tests.cpp 157a56aa06677d8b7a2cef53b29ed05cb4b5d8ea > > Diff: https://reviews.apache.org/r/36816/diff/ > > > Testing > ------- > > * Add a new unit test: HealthCheckTest.HealthyTaskThroughHttp > make check > > > Thanks, > > haosdent huang > >
