----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41154/#review109859 -----------------------------------------------------------
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/test/python/apache/aurora/common/health_check/__init__.py` src/main/python/apache/aurora/common/health_check/shell.py (line 55) <https://reviews.apache.org/r/41154/#comment169518> I don't think this is true. OSError can signal a variety of underlying issues. I think it would be better to set the reason to "OSError: {error}".format(e.strerror). 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) <https://reviews.apache.org/r/41154/#comment169524> 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 `src/main/python/apache/aurora/client/config.py`. 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" field. 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 > 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 > >
