> On March 31, 2016, 4:20 p.m., Dmitriy Shirchenko wrote:
> > src/main/python/apache/aurora/common/health_check/shell.py, line 60
> > <https://reviews.apache.org/r/45506/diff/2/?file=1320188#file1320188line60>
> >
> >     why did you get rid of .format? i personally find it much clearer to 
> > understand so would you please justify this decision.
> >     
> >     some refs:
> >     http://stackoverflow.com/a/5082482
> >     talk of deprecating % altogether in python3 and explicit mention for 
> > using .format.
> >     https://docs.python.org/3/whatsnew/2.6.html#pep-3101
> 
> Bill Farner wrote:
>     That may be true, but `%` is still the predominant pattern in this 
> codebase for string formatting.
>     ```console
>     $ grep -R '.format(' src/{main,test}/python | wc -l
>            3
>     $ grep -R ' % ' src/{main,test}/python | wc -l
>          785
>     ```
>     I tend to follow the convention until a mass switch takes place.  Patches 
> welcome :-)

I see your point. My point is that objectively .format is the correct way 
(given refs ^). I don't feel strongly about it, but I will say that I don't 
agree with logic of going down the incorrect path as it will make it difficult 
(or impossible) to correct the behavior down the line. In all of my patches, I 
use .format but it looks like I am getting overruled and using .format is 
discouraged :). It's not a big deal!


> On March 31, 2016, 4:20 p.m., Dmitriy Shirchenko wrote:
> > src/main/python/apache/aurora/common/health_check/shell.py, line 66
> > <https://reviews.apache.org/r/45506/diff/2/?file=1320188#file1320188line66>
> >
> >     why are you doing a separate try/except with timeout when 
> > subprocess.Popen already supports it.
> >     
> >     you can still do a kill if that's your intention in one swoop.
> 
> Bill Farner wrote:
>     I'm generally a fan of scoping exceptions to catch only where they are 
> known to be thrown.  If `Popen.__init__` and `Popen.wait` threw the same 
> exception type, i would probably put them in the same `try`, but in this case 
> they are distinct.
>     
>     However, i'm not sure what you mean by 'Popen already supports it', so 
> it's possible i'm missing something.

If you look at subprocess32, timeout is an option into Popen. What's the logic 
behind changing how timeout is done via process.wait instead of relying on 
Popen()?


- Dmitriy


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


On March 31, 2016, 6:38 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45506/
> -----------------------------------------------------------
> 
> (Updated March 31, 2016, 6:38 p.m.)
> 
> 
> Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
> 
> 
> Bugs: AURORA-1641
>     https://issues.apache.org/jira/browse/AURORA-1641
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Here's a stab at this using `os` and `pwd` modules directly to demote health 
> checks to the target user.
> 
> 
> Diffs
> -----
> 
>   src/main/python/apache/aurora/common/health_check/shell.py 
> 6cb7dfc164f4e16143fc974d50c19a5887d32015 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a 
>   src/test/python/apache/aurora/common/health_check/test_shell.py 
> 7026af8c4671a40f4b517ecf12149eac34a552c8 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 19c4f76347e34374c29974c182d1f4c118bcb18d 
> 
> Diff: https://reviews.apache.org/r/45506/diff/
> 
> 
> Testing
> -------
> 
> I haven't spent any time thinking of a test strategy for this, but i don't 
> think we should proceed without end-to-end validation.  I'm open to ideas 
> here.
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Reply via email to