> On Dec. 10, 2015, 8:23 p.m., Bill Farner wrote: > > docs/configuration-reference.md, line 394 > > <https://reviews.apache.org/r/41154/diff/2/?file=1157822#file1157822line394> > > > > How about `shell_command` instead?
Done. > On Dec. 10, 2015, 8:23 p.m., Bill Farner wrote: > > src/main/python/apache/aurora/common/health_check/generic.py, line 1 > > <https://reviews.apache.org/r/41154/diff/2/?file=1157826#file1157826line1> > > > > Similar to comment above - 'generic' is non-descriptive, consider using > > 'shell' and/or 'command' in place of 'generic' throughout. Done. > On Dec. 10, 2015, 8:23 p.m., Bill Farner wrote: > > src/main/python/apache/aurora/common/health_check/generic.py, line 49 > > <https://reviews.apache.org/r/41154/diff/2/?file=1157826#file1157826line49> > > > > Forgive me for being lazy, quick search didn't reveal an answer - how > > does a timeout surface? is this what you are looking for? https://github.com/google/python-subprocess32/blob/master/subprocess32.py#L580 > On Dec. 10, 2015, 8:23 p.m., Bill Farner wrote: > > src/main/python/apache/aurora/common/health_check/generic.py, line 52 > > <https://reviews.apache.org/r/41154/diff/2/?file=1157826#file1157826line52> > > > > s/Our/The/ Done. > On Dec. 10, 2015, 8:23 p.m., Bill Farner wrote: > > src/main/python/apache/aurora/common/health_check/generic.py, line 61 > > <https://reviews.apache.org/r/41154/diff/2/?file=1157826#file1157826line61> > > > > What's being caught here? We generally avoid broad catches like this, > > so ideally we would constrain it more. I was a little lazy in tracking exceptions, but thanks for making me research more. Changing to ValueError as the remaining documented exception that could be thrown by check_call and Popen via https://github.com/google/python-subprocess32/blob/master/subprocess32.py#L169 > On Dec. 10, 2015, 8:23 p.m., Bill Farner wrote: > > src/main/python/apache/aurora/common/health_check/generic.py, line 53 > > <https://reviews.apache.org/r/41154/diff/2/?file=1157826#file1157826line53> > > > > 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. I think the str(reason) here is the stderr that we are looking for and it's not too verbose. For example, this will return something like ``` CalledProcessError: Command '['grep', 'foo']' returned non-zero exit status 2 ```. Here's the code for the CalledProcessError (actually an Exception): https://github.com/google/python-subprocess32/blob/master/subprocess32.py#L428 ``` def __str__(self): return "Command '%s' returned non-zero exit status %d" % (self.cmd, self.returncode) ``` This is very short and sweet, though maybe user would benefit from `output`. Do you feel like this is reasonable? - Dmitriy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41154/#review109847 ----------------------------------------------------------- On Dec. 10, 2015, 12:14 a.m., Dmitriy Shirchenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41154/ > ----------------------------------------------------------- > > (Updated Dec. 10, 2015, 12:14 a.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 > >
