This is an automatically generated e-mail. To reply, visit:

I have not looked at the tests yet but the implementation looks ok to me. I 
also agree with the idea in general. I think having a more flexible concept of 
health checking is a good feature to have.

Also I think a header needs to be in 

src/main/python/apache/aurora/common/health_check/shell.py (line 55)

    I don't think this is true. OSError can signal a variety of underlying 
    I think it would be better to set the reason to "OSError: 
    For example the binary might not be found, but it also might not be 
executable by the current user, etc, etc.

src/main/python/apache/aurora/executor/common/health_checker.py (line 212)

    It seems like a logical error to both set "shell_command" and have the 
'health' port set.
    I think we should raise an error on the client side and inform the user the 
schema is invalid so the logic here can remain the same. You can do this by 
adding another check to `validate_config` in 
    Also, it seems bit weird to have the switch between http checking and shell 
checking to be the presence of the health port vs setting the "shell_command" 
    What do you think about having a type field to the HealthCheck config that 
can be one of 'http' (default) or 'shell'. The documentation can then be 
updated to explain which fields are expected for each type. I think this might 
make it easier to add more types of health checking in the future.

- Zameer Manji

On Dec. 10, 2015, 1:16 p.m., Dmitriy Shirchenko wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41154/
> -----------------------------------------------------------
> (Updated Dec. 10, 2015, 1:16 p.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/common/BUILD 
> 5fce3d0d29d2a38c6563b4d9be963532e595ee19 
>   src/main/python/apache/aurora/common/health_check/BUILD PRE-CREATION 
>   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/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 
>   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