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 51069: Refactored `_commandHealthCheck` in health check.
Date Mon, 15 Aug 2016 00:37:44 GMT

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


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