----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36816/#review146941 -----------------------------------------------------------
Fix it, then Ship it! src/health-check/health_checker.cpp (lines 69 - 70) <https://reviews.apache.org/r/36816/#comment213830> Blank line src/health-check/health_checker.cpp (lines 70 - 71) <https://reviews.apache.org/r/36816/#comment213833> // Use '127.0.0.1' instead of 'localhost', because the host // file in some container images may not contain 'localhost'. src/health-check/health_checker.cpp (line 71) <https://reviews.apache.org/r/36816/#comment213832> typo: container src/health-check/health_checker.cpp (lines 333 - 337) <https://reviews.apache.org/r/36816/#comment213837> How about this for readability: ``` string scheme = http.has_scheme() ? http.scheme() : DEFAULT_HTTP_SCHEME; string path = http.has_path() ? http.path() : ""; string url = scheme + "://" + DEFAULT_DOMAIN + ":" + stringify(http.port()) + path; ``` src/health-check/health_checker.cpp (line 345) <https://reviews.apache.org/r/36816/#comment213839> I'm not sure whether this is good or not, let's leave it for now. If there will be user feedback, we can fix it later. src/health-check/health_checker.cpp (line 360) <https://reviews.apache.org/r/36816/#comment213840> s/exec/create src/health-check/health_checker.cpp (line 378) <https://reviews.apache.org/r/36816/#comment213841> "Killing the HTTP health check process " src/health-check/health_checker.cpp (line 384) <https://reviews.apache.org/r/36816/#comment213842> "curl has not returned after " + stringify(timeout) + "; aborting" src/health-check/health_checker.cpp (lines 390 - 392) <https://reviews.apache.org/r/36816/#comment213843> For readability, let's reformat like in the declaration: ``` const std::tuple< process::Future<Option<int>>, process::Future<std::string>, process::Future<std::string>>& t); ``` src/health-check/health_checker.cpp (line 396) <https://reviews.apache.org/r/36816/#comment213844> Let's be consistent and call it either subprocess or process. I'd vote for "process". - Alexander Rukletsov On Aug. 25, 2016, 1:24 p.m., haosdent huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36816/ > ----------------------------------------------------------- > > (Updated Aug. 25, 2016, 1:24 p.m.) > > > Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón > Kleiman, 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 > b4548f385e6bdf12f6bbc402a5d59ba8e165b8a5 > src/health-check/health_checker.cpp > 45a5fe00a95a6e88b1990c1396e03082feb202bc > > Diff: https://reviews.apache.org/r/36816/diff/ > > > Testing > ------- > > > Thanks, > > haosdent huang > >
