----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51069/#review145731 -----------------------------------------------------------
Looks great, thanks for cleaning things up! I'm not yet convinced we should add heavy machinery to get access to the command's stderr. If I understood correctly, you need it _just_ to print extra message in case health check fails. src/health-check/health_checker.cpp (line 144) <https://reviews.apache.org/r/51069/#comment212090> Let's promote it to `LOG(WARNING)` for now. If it's too much, we will reconsider. src/health-check/health_checker.cpp (line 190) <https://reviews.apache.org/r/51069/#comment212082> `checkResult`? src/health-check/health_checker.cpp (line 224) <https://reviews.apache.org/r/51069/#comment212084> s/msg/message src/health-check/health_checker.cpp (line 247) <https://reviews.apache.org/r/51069/#comment212091> Not yours, but I'd argue wrapping it in `Optional` is misleading. There is no way we can get out of here without creating a process. I suggest we change it to smth like: `Try<Subprocess> s = Error("Not launched");` src/health-check/health_checker.cpp (lines 302 - 303) <https://reviews.apache.org/r/51069/#comment212092> Let's swap these lines and rename `Kill` to `Killing` src/health-check/health_checker.cpp (line 307) <https://reviews.apache.org/r/51069/#comment212089> I don't think we need "Aborting because" part. Let's think about how the resulting message will look like: "COMMAND health check failed: Command has not returned after ". Does it look good from a user perspective? src/health-check/health_checker.cpp (line 315) <https://reviews.apache.org/r/51069/#comment212093> It's not a `future`, `tuple` at best : ) It looks like we call it `t` in such cases, which is not the best name. If you can come up with a better name, this would be great, otherwise feel free to use `t`. src/health-check/health_checker.cpp (line 320) <https://reviews.apache.org/r/51069/#comment212094> s/subprocess// src/health-check/health_checker.cpp (line 325) <https://reviews.apache.org/r/51069/#comment212095> s/subprocess/process src/health-check/health_checker.cpp (line 328) <https://reviews.apache.org/r/51069/#comment212096> let's extract status code into a var `statusCode`. We use it several times later. src/health-check/health_checker.cpp (lines 331 - 333) <https://reviews.apache.org/r/51069/#comment212098> How about: "Command returned " + WSTRINGIFY(statusCode) + "; reading stderr failed: " + (error.isFailed() ? error.failure() : "discarded"); src/health-check/health_checker.cpp (lines 336 - 337) <https://reviews.apache.org/r/51069/#comment212097> How about "Command returned " + WSTRINGIFY(statusCode) + ": " + error.get(); src/health-check/health_checker.cpp (line 349) <https://reviews.apache.org/r/51069/#comment212083> If we do not fail the promise, an executor may retry. I think we want to communicate to the executor that there is no way HTTP (or TCP) health check will work. - Alexander Rukletsov On Aug. 14, 2016, 5:04 p.m., haosdent huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51069/ > ----------------------------------------------------------- > > (Updated Aug. 14, 2016, 5:04 p.m.) > > > Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón > Kleiman, Gilbert Song, Jie Yu, and Timothy Chen. > > > Repository: mesos > > > Description > ------- > > * Removed blocking `Future::await` call. > * Adjust the level of some logs. > * Adjust some minor styles. > * Change the interfaces of different health check handlers to > `Future<Nothing>` to make errors handling more easier. > > > Diffs > ----- > > src/health-check/health_checker.hpp > b4548f385e6bdf12f6bbc402a5d59ba8e165b8a5 > src/health-check/health_checker.cpp > 45a5fe00a95a6e88b1990c1396e03082feb202bc > > Diff: https://reviews.apache.org/r/51069/diff/ > > > Testing > ------- > > > Thanks, > > haosdent huang > >
