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


Overall this looks great!  Focusing on 2 nits in this pass - naming, and 
handling command output.


docs/configuration-reference.md (line 390)
<https://reviews.apache.org/r/41154/#comment169493>

    How about `shell_command` instead?



src/main/python/apache/aurora/common/health_check/generic.py (line 1)
<https://reviews.apache.org/r/41154/#comment169494>

    Similar to comment above - 'generic' is non-descriptive, consider using 
'shell' and/or 'command' in place of 'generic' throughout.



src/main/python/apache/aurora/common/health_check/generic.py (line 18)
<https://reviews.apache.org/r/41154/#comment169495>

    Meta: thanks a lot for the comment, very helpful.



src/main/python/apache/aurora/common/health_check/generic.py (line 49)
<https://reviews.apache.org/r/41154/#comment169497>

    Forgive me for being lazy, quick search didn't reveal an answer - how does 
a timeout surface?



src/main/python/apache/aurora/common/health_check/generic.py (line 52)
<https://reviews.apache.org/r/41154/#comment169496>

    s/Our/The/



src/main/python/apache/aurora/common/health_check/generic.py (line 53)
<https://reviews.apache.org/r/41154/#comment169498>

    We probably need more thought here w.r.t. the output.  For example, in most 
cases we probably want stderr/stdout to surface to the user (via the 
scheduler).  However, we should guard a bit and truncate that output to avoid 
lots of garbage filling up the scheduler's memory.
    
    So, summary - please make sure stderr/stdout (and probably exit code) are 
returned, but unbounded output is truncated.



src/main/python/apache/aurora/common/health_check/generic.py (line 61)
<https://reviews.apache.org/r/41154/#comment169499>

    What's being caught here?  We generally avoid broad catches like this, so 
ideally we would constrain it more.


- Bill Farner


On Dec. 9, 2015, 4:14 p.m., Dmitriy Shirchenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41154/
> -----------------------------------------------------------
> 
> (Updated Dec. 9, 2015, 4:14 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/generic.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_generic.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