> On Dec. 10, 2015, 10:11 p.m., Joshua Cohen wrote:
> > src/main/python/apache/aurora/common/health_check/BUILD, lines 1-28
> > <https://reviews.apache.org/r/41154/diff/3/?file=1159072#file1159072line1>
> >
> >     I don't think this BUILD file should exist. Per our [python build 
> > conventions](https://github.com/apache/aurora/blob/master/docs/build-system.md#python-build-conventions)
> >  we should have a single BUILD at the 3rdlevel directory (common) in this 
> > case, that includes everything below via rglobs. Note that other 4th and 
> > lower level directories do no not have BUILD files:
> >     
> >         $ find src/main/python -maxdepth 99 -mindepth 4 -type d |xargs -I 
> > {} find {} -name BUILD
> >         src/main/python/apache/aurora/client/cli/BUILD
> >         
> >         $ cat src/main/python/apache/aurora/client/cli/BUILD
> >         
> >         # License header omitted
> >         python_library(
> >           name = 'auroraversion',
> >           resources = [ '.auroraversion' ]
> >         )
> >     
> >     So the only build file that exists below 3 levels deep exists to pull 
> > in .auroraversion as a cli resource.
> >     
> >     If I'm wrong and this BUILD file *should* exist, then at the very least 
> > the c/p from the directory above should be fixed so the name is 
> > `health_check` and the name of the provided library does not duplicate 
> > apache.aurora.common.

Will delete. Thanks for feedback!


> On Dec. 10, 2015, 10:11 p.m., Joshua Cohen wrote:
> > src/main/python/apache/aurora/common/health_check/shell.py, line 49
> > <https://reviews.apache.org/r/41154/diff/3/?file=1159074#file1159074line49>
> >
> >     I second Steve's concern about adding a dependency on subprocess32. Can 
> > you clarify why it's needed? subprocess.check_call is already available in 
> > python2.7 stdlib: 
> > https://docs.python.org/2/library/subprocess.html#subprocess.check_call.
> 
> Joshua Cohen wrote:
>     Nevermind this, discussion above happened while I was reviewing :).

https://docs.python.org/2/library/subprocess.html#subprocess.check_call doesn't 
support timeouts. 
Compare to:
https://github.com/google/python-subprocess32/blob/master/subprocess32.py#L577 
or 3+ version: 
https://docs.python.org/3/library/subprocess.html#subprocess.check_call


- Dmitriy


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


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
> 
>

Reply via email to