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



src/main/python/apache/aurora/config/schema/base.py (line 46)
<https://reviews.apache.org/r/41154/#comment169813>

    I second Zameer's concerns about logical conflict between the new 
`shell_command` and the current http-based config values. I strongly suggest 
considering an approach similar to `LifecycleConfig` to avoid contaminating 
health check config with logically conflicting knobs. Something on the lines of 
(not focusing on names):
    
    ```
    HttpEndpointConfig(Struct):
      endpoint = ...
      expected_response = ...
      expected_response_code = ...
      
    ShellEndpointConfig(Struct):
      shell_command = ...
      
    HealthEndpointConfig(struct):
      http = HttpEndpointConfig
      shell = ShellEndpointConfig
      
    DefaultEndpointConfig = HealthEndpointConfig(http = HttpEndpointConfig())
    
    HealthCheckConfig(Struct):
      ...
      endpoint_config = Default(HealthEndpointConfig, DefaultEndpointConfig)
    ```
    
    Now, the above would require deprecating the existing fields in the 
HealthCheckConfig (those that will move into HttpEndpointConfig) but longer 
term (e.g. in 0.12.0) we will be able to remove (or alias) those and have a 
much cleaner schema open for adding any kinds of other health checkers.



src/main/python/apache/aurora/config/schema/base.py (line 47)
<https://reviews.apache.org/r/41154/#comment169793>

    Please, don't use 'type' for a field name. It's too generic and clashes 
with python defaults.


- Maxim Khutornenko


On Dec. 11, 2015, 7:25 a.m., Dmitriy Shirchenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41154/
> -----------------------------------------------------------
> 
> (Updated Dec. 11, 2015, 7:25 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1551
>     https://issues.apache.org/jira/browse/AURORA-1551
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Adding support for non-HTTP health checks.
> 
> 
> Diffs
> -----
> 
>   3rdparty/python/requirements.txt cfef18ee66b0f92d83c53dacb6d9376fc2e50445 
>   docs/configuration-reference.md 364292998bebb233d300fe59c9ea42b216deee81 
>   src/main/python/apache/aurora/client/config.py 
> 2fc12559016d406c347adb416a5166cca31c961e 
>   src/main/python/apache/aurora/common/BUILD 
> 5fce3d0d29d2a38c6563b4d9be963532e595ee19 
>   src/main/python/apache/aurora/common/health_check/__init__.py PRE-CREATION 
>   src/main/python/apache/aurora/common/health_check/shell.py PRE-CREATION 
>   src/main/python/apache/aurora/common/http_signaler.py 
> a3193f3259276ec23d37f45839afe3c387cff6b1 
>   src/main/python/apache/aurora/config/schema/base.py 
> 398f737bed9ef02ce4a5636896d6587bce26501e 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> 03fdf0afef120c365c6ffad09e152780eed7e351 
>   src/main/python/apache/aurora/executor/http_lifecycle.py 
> 6d578cceb56375425ccac1cbfbbcd0add60f20e9 
>   src/test/python/apache/aurora/client/BUILD 
> 84c5c845d9b1c8078ae8b47242d0f2ffc00ef6dc 
>   src/test/python/apache/aurora/client/test_config.py 
> b1a3c1865819899ef19173be0f861783a2631d0a 
>   src/test/python/apache/aurora/common/BUILD 
> 2556c32842b3cf7040cb3c41172a0d9c365cb649 
>   src/test/python/apache/aurora/common/health_check/BUILD PRE-CREATION 
>   src/test/python/apache/aurora/common/health_check/__init__.py PRE-CREATION 
>   src/test/python/apache/aurora/common/health_check/test_shell.py 
> PRE-CREATION 
>   src/test/python/apache/aurora/common/test_http_signaler.py 
> f68c71a6765f7f0b93c8c50662515b5742344f35 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 27c71711d52f757ed1552db4accda671a6bdafdd 
> 
> Diff: https://reviews.apache.org/r/41154/diff/
> 
> 
> Testing
> -------
> 
> Added unit tests. 
> Ran e2e test.
> Tested expected behavior on virtual Mesos cluster.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Reply via email to