> On Dec. 10, 2015, 9:54 p.m., Zameer Manji wrote: > > src/main/python/apache/aurora/executor/common/health_checker.py, line 212 > > <https://reviews.apache.org/r/41154/diff/3/?file=1159077#file1159077line212> > > > > 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.
Great idea. Adding! > On Dec. 10, 2015, 9:54 p.m., Zameer Manji wrote: > > src/main/python/apache/aurora/common/health_check/shell.py, line 55 > > <https://reviews.apache.org/r/41154/diff/3/?file=1159074#file1159074line55> > > > > 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. Done. - Dmitriy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41154/#review109859 ----------------------------------------------------------- On Dec. 10, 2015, 9: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, 9: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 > >
