----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56208/#review165521 -----------------------------------------------------------
Many of the comments here also apply to the current code in `health_checker.cpp`. One possibility would be to commit this patch without fixing them, and then creating a new patch with the fixes for both `health_checker.cpp` and `checker.cpp`. src/checks/checker.hpp (line 61) <https://reviews.apache.org/r/56208/#comment237374> Do we need to pass the `TaskID` to the callback? The `checker` is bound to a task, so the creator can do something like: ``` checker = Checker::create( checkInfo, defer(self(), &Self::checkStatusUpdated, taskId, lambda::_1), ... ); ``` src/checks/checker.hpp (line 77) <https://reviews.apache.org/r/56208/#comment237361> `Checker(const Checker&) = delete` seems to be more popular than making the constructor private. Is that idiom preferred? src/checks/checker.cpp (line 31) <https://reviews.apache.org/r/56208/#comment237366> Do we really need `startTime` (see two issues below)? If not, we can remove this. src/checks/checker.cpp (line 65) <https://reviews.apache.org/r/56208/#comment237367> Ditto. src/checks/checker.cpp (line 173) <https://reviews.apache.org/r/56208/#comment237364> Looks like a left-over from health checks. There it is used for the grace period, but here it is initialized, and then never used again. src/checks/checker.cpp (line 219) <https://reviews.apache.org/r/56208/#comment237379> I think that it'd be useful to log the `taskId` here, since an executor might have multiple checkers running at the same time. src/checks/checker.cpp (line 267) <https://reviews.apache.org/r/56208/#comment237378> Ditto `taskId`. src/checks/checker.cpp (line 305) <https://reviews.apache.org/r/56208/#comment237377> Ditto `taskId`. src/checks/checker.cpp (line 315) <https://reviews.apache.org/r/56208/#comment237380> Ditto `taskId`. src/checks/checker.cpp (lines 339 - 344) <https://reviews.apache.org/r/56208/#comment237384> I know that what you put here is what we currently do for command health checks, but it would probably make sense to overwrite env variables set in `check.command().command().environment()`. What we have here seems to be the equivalent of passing `environment = None()` to `subprocess()`. src/checks/checker.cpp (line 351) <https://reviews.apache.org/r/56208/#comment237381> Ditto `taskId`. src/checks/checker.cpp (line 365) <https://reviews.apache.org/r/56208/#comment237382> Ditto `taskId`. src/checks/checker.cpp (line 383) <https://reviews.apache.org/r/56208/#comment237387> mark this `const`? or is it considered a POD? src/checks/checker.cpp (line 394) <https://reviews.apache.org/r/56208/#comment237388> Ditto `taskId`. src/checks/checker.cpp (line 420) <https://reviews.apache.org/r/56208/#comment237389> Ditto `taskId`. src/checks/checker.cpp (line 428) <https://reviews.apache.org/r/56208/#comment237390> Ditto `taskId`. src/checks/checker.cpp (line 450) <https://reviews.apache.org/r/56208/#comment237391> Ditto `taskId`. src/checks/checker.cpp (line 481) <https://reviews.apache.org/r/56208/#comment237392> ditto marking it as `const` src/checks/checker.cpp (line 497) <https://reviews.apache.org/r/56208/#comment237393> Ditto `taskId`. src/checks/checker.cpp (line 570) <https://reviews.apache.org/r/56208/#comment237395> Ditto `taskId`. - Gastón Kleiman On Feb. 9, 2017, 12:56 p.m., Alexander Rukletsov wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56208/ > ----------------------------------------------------------- > > (Updated Feb. 9, 2017, 12:56 p.m.) > > > Review request for mesos, Gastón Kleiman and Vinod Kone. > > > Bugs: MESOS-6906 > https://issues.apache.org/jira/browse/MESOS-6906 > > > Repository: mesos > > > Description > ------- > > Add support for general checks, i.e. defined by CheckInfo, in > checking library. A general check can be either an command or > an HTTP request. The library performs the requested check at > the specified interval and sends the result to the framework > via a task status update. If the current result is the same as > the previous result, no status update is sent. > > > Diffs > ----- > > src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277 > src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6 > src/checks/health_checker.hpp f1f2834b3429fb00cc49c179fa9a3de328f597b5 > src/checks/health_checker.cpp a5225ff1f4b071ed4182d41fa8ecc705fa4dbe00 > > Diff: https://reviews.apache.org/r/56208/diff/ > > > Testing > ------- > > See https://reviews.apache.org/r/56218/ > > > Thanks, > > Alexander Rukletsov > >
