mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 51560: Deprecated using health checks without setting the type.
Date Mon, 26 Sep 2016 17:22:23 GMT

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




src/health-check/health_checker.cpp (lines 185 - 189)
<https://reviews.apache.org/r/51560/#comment218350>

    How about the following?
    
    ```
    if (check.has_command() && !check.has_http()) {
      check.set_type(HealthCheck::COMMAND);
    } else if (check.has_http() && !check.has_command()) {
      check.set_type(HealthCheck::HTTP);
    } else {
    ...
    }
    ```
    
    It's obviously problematic to specify both but here we need to ensure that the behavior
doesn't depend on the fact we look at `has_command()` first.
    
    The other thing is that it not ideal that we need to do this in two different places.
In the codebase we have been more consistently doing the following:
    
    1. Fill in missing fields for backwards compatibility and then
    2. Keep the rest of the code free from such concerns.
    
    [One example](https://github.com/apache/mesos/blob/ec4c81a12559030791334359e7e1e2b6565cce01/src/master/master.cpp#L4066)
    
    Logically this block of code could be put directly below this example, i.e., just before
task validation.
    
    ```
    TaskInfo task_(task);
    if (task.has_executor() && !task.executor().has_framework_id()) {
        task_.mutable_executor()
            ->mutable_framework_id()->CopyFrom(framework->id());
    }
    
    if (check.has_command()) {
      check.set_type(HealthCheck::COMMAND);
    } else if (check.has_http()) {
      check.set_type(HealthCheck::HTTP);
    }
    ```
    
    Furthermore, it would be better if we extract these lines into a method.
    
    ```
    TaskInfo adapt(const TaskInfo& task);
    ````
    
    which takes care of all (past and future) such adjustments. I am not sure if devolve is
the right place and we can put a TODO here and spend more time thinking about it outside this
RR.



src/tests/health_check_tests.cpp (line 212)
<https://reviews.apache.org/r/51560/#comment218351>

    s/compatibility reasons/backwards compatibility/ so it's more clear.



src/tests/health_check_tests.cpp (line 226)
<https://reviews.apache.org/r/51560/#comment218352>

    s/compatibility reasons/backwards compatibility/ so it's more clear.


- Jiang Yan Xu


On Sept. 23, 2016, 8:38 a.m., haosdent huang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51560/
> -----------------------------------------------------------
> 
> (Updated Sept. 23, 2016, 8:38 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joseph Wu, Silas Snider, and Jiang Yan
Xu.
> 
> 
> Bugs: MESOS-6110
>     https://issues.apache.org/jira/browse/MESOS-6110
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Health checks must supply set type field from now on. Additionally,
> `HealthCheck.HTTP` message has been renamed to
> `HealthCheck.HttpCheckInfo` to avoid naming collisions.
> 
> 
> Diffs
> -----
> 
>   src/health-check/health_checker.cpp 736725c4ef954ece8580f383cfd31d289795903f 
>   src/tests/health_check_tests.cpp e6b02f23dc1b1b84381ab0af0e3df5918b60ae40 
> 
> Diff: https://reviews.apache.org/r/51560/diff/
> 
> 
> Testing
> -------
> 
> Could verfied from https://github.com/haosdent/mesos/blob/85abb6da058bf8daf4e794df7da8eb5f240b671c/docs/upgrades.md
> 
> 
> Thanks,
> 
> haosdent huang
> 
>


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