mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gastón Kleiman <gas...@mesosphere.io>
Subject Re: Review Request 56208: Updated checks library with general check support.
Date Tue, 14 Feb 2017 17:46:47 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message