> On Dec. 10, 2015, 9:23 p.m., Steve Niemitz wrote:
> > Overall comment here: is there a really good reason to introduce another 
> > third party library (subprocess32)?  Especially because its not pure python 
> > (there's a C extension).  I'd prefer to see this done without introducing 
> > other dependencies.
> 
> Zameer Manji wrote:
>     Steve, you bring up a good point here. The addition of a non pure Python 
> 3rdparty dependency does increase the build complexity. Dmitriy, what are 
> your thoughts on just using the subprocess module in Python2.7? Bill, Maxim, 
> what are your thoughts here on that?

Yea, I completely agree that adding dependencies aren't great. 

In this case, I would make a claim that subprocess32 (called subprocess in 3+) 
is already a part of standard library in Python 3+ so I feel like this is not 
an unreasonable to include it. 

subprocess32 in particular claims to fix concurrency bugs [1] and should be 
used for Python 2.7. Also, subprocess in 2.7 would not support timeout option 
so we would have to take out that feature in exchange for using 2.7 subprocess. 

I am open to taking out timeout feature if we really do not want to add 
subprocess32, though it sounds like we should be using subprocess32 instead of 
subprocess anyway [if Google's claims are true). :)

[1]
https://github.com/google/python-subprocess32
subprocess32 includes many important reliability bug fixes relevant on POSIX 
platforms. The most important of which is a C extension module used internally 
to handle the code path between fork() and exec(). This module is reliable when 
an application is using threads.


- Dmitriy


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


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