> On Dec. 10, 2015, 1: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?
> 
> Dmitriy Shirchenko wrote:
>     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.

Google's claims aside, I think the ability to use timeout is important. I'm now 
fine with the dependency since it allows for the timeout functionality. Thanks 
for diving into this.


- Zameer


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


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

Reply via email to