mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rukletsov <ruklet...@gmail.com>
Subject Re: Review Request 59873: Refactored `HealthChecker` to use `CheckerProcess`.
Date Thu, 08 Jun 2017 16:33:24 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59873/#review177306
-----------------------------------------------------------




src/checks/health_checker.hpp
Line 39 (original), 32-33 (patched)
<https://reviews.apache.org/r/59873/#comment250848>

    Insert a blank line please.



src/checks/health_checker.cpp
Lines 255-256 (patched)
<https://reviews.apache.org/r/59873/#comment250849>

    I would still do it like we do it in `CheckerProcess`.



src/checks/health_checker.cpp
Line 199 (original), 263-264 (patched)
<https://reviews.apache.org/r/59873/#comment250852>

    I think we can do it `CheckerProcess`



src/checks/health_checker.cpp
Lines 222-271 (original), 310-326 (patched)
<https://reviews.apache.org/r/59873/#comment250836>

    How about this:
    ```
    void HealthChecker::processCheckResult(const Try<CheckStatusInfo>& result)
    {
      if (result.isError()) {
        // The error is with the underlying check.
        LOG(WARNING) << name << " for task '" << taskId << "'" <<
" failed: "
                     << result.error();
    
        failure();
        return;
      }
    
      Try<Nothing> healthCheckResult = interpretCheckStatusInfo(result.get());
      if (healthCheckResult.isError()) {
        // The underlying check succeeded, but its result is interpreted as failure.
        LOG(WARNING) << name << " for task '" << taskId << "'" <<
" failed: "
                     << healthCheckResult.error();
    
        failure();
        return;
      }
    
      success();
    }
    ```
    ? Note the changed signature of `failure()`.



src/checks/health_checker.cpp
Lines 249-251 (original), 313-315 (patched)
<https://reviews.apache.org/r/59873/#comment250827>

    We can profit from using `name` here.



src/checks/health_checker.cpp
Lines 249-251 (original), 313-315 (patched)
<https://reviews.apache.org/r/59873/#comment250834>

    This contains a lot of duplicated text with `failure()`.



src/checks/health_checker.cpp
Lines 290-292 (original), 335-338 (patched)
<https://reviews.apache.org/r/59873/#comment250828>

    We can profit from using `name` here as well.



src/checks/health_checker.cpp
Lines 298-300 (original), 343-345 (patched)
<https://reviews.apache.org/r/59873/#comment250835>

    Here we should probably print `consecutiveFailures`, because a health check may fail consecutively
for various reasons.



src/checks/health_checker.cpp
Lines 388-392 (patched)
<https://reviews.apache.org/r/59873/#comment250853>

    We don't validate other `Duration`s this way neither here not for `CheckInfo`. Can we
do it consistently or update all occurences? Also, pleasa not in this patch : )


- Alexander Rukletsov


On June 7, 2017, 11:56 p.m., Gastón Kleiman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59873/
> -----------------------------------------------------------
> 
> (Updated June 7, 2017, 11:56 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Bugs: MESOS-7092
>     https://issues.apache.org/jira/browse/MESOS-7092
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> `HealthChecker` now wraps a `CheckerProcess` and gets check status
> updates via a callback.
> 
> 
> Diffs
> -----
> 
>   src/checks/checker.cpp dcc3164f3b623de73bacf024ede95b40c48f7192 
>   src/checks/checker_process.hpp PRE-CREATION 
>   src/checks/checker_process.cpp PRE-CREATION 
>   src/checks/health_checker.hpp 25bf7e99bf5982fd7a6ff5012c231ff89fb7cb39 
>   src/checks/health_checker.cpp 9d8c8471caa05af3908d34315dbfed08a343c0f8 
> 
> 
> Diff: https://reviews.apache.org/r/59873/diff/2/
> 
> 
> Testing
> -------
> 
> Tests still pass on GNU/Linux.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>


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