Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-30 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/#review151050 --- Ship it! Master (59b4d31) is green with this patch.

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-30 Thread Joshua Cohen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/#review151045 --- Ship it! Ship It! - Joshua Cohen On Sept. 30, 2016, 5:17

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-30 Thread Kai Huang
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/ --- (Updated Sept. 30, 2016, 5:17 p.m.) Review request for Aurora, Joshua Cohen

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-30 Thread Joshua Cohen
> On Sept. 30, 2016, 4:11 p.m., Joshua Cohen wrote: > > Can you add a test case for the scenario where a status provider throws > > during initialization? > > Kai Huang wrote: > Oh, that's a good point. Will do. Thanks. e2e's passed for me locally w/ this patch, so once the tests are

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-30 Thread Kai Huang
> On Sept. 30, 2016, 4:11 p.m., Joshua Cohen wrote: > > Can you add a test case for the scenario where a status provider throws > > during initialization? Oh, that's a good point. Will do. - Kai --- This is an automatically generated

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-30 Thread Joshua Cohen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/#review151029 --- Can you add a test case for the scenario where a status provider

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-29 Thread Kai Huang
> On Sept. 28, 2016, 9:52 p.m., Joshua Cohen wrote: > > I tried to commit this, but e2e tests hung for me. > > > > Kai, can you investigate? > > Kai Huang wrote: > There is a bug of thermos kill for task running in docker > container(https://issues.apache.org/jira/browse/AURORA-1426 ). It

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-29 Thread Kai Huang
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/ --- (Updated Sept. 29, 2016, 11:11 p.m.) Review request for Aurora, Joshua Cohen

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-29 Thread Kai Huang
> On Sept. 28, 2016, 9:52 p.m., Joshua Cohen wrote: > > I tried to commit this, but e2e tests hung for me. > > > > Kai, can you investigate? There is a bug of thermos kill for task running in docker container(https://issues.apache.org/jira/browse/AURORA-1426 ). It is likely due to my code

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-28 Thread Joshua Cohen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/#review150769 --- I tried to commit this, but e2e tests hung for me. Kai, can you

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-28 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/#review150764 --- Ship it! Master (1c1c0a5) is green with this patch.

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-28 Thread Kai Huang
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/ --- (Updated Sept. 28, 2016, 9:07 p.m.) Review request for Aurora, Joshua Cohen

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-27 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/#review150644 --- Ship it! Master (69cba78) is green with this patch.

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-27 Thread Kai Huang
> On Sept. 27, 2016, 9:27 p.m., Stephan Erb wrote: > > src/main/python/apache/aurora/executor/common/status_checker.py, line 104 > > > > > > This is slightly beyond the pull request, but for the matter of > >

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-27 Thread Stephan Erb
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/#review150612 --- Ship it! LGTM. A couple of minor remarks below. I am late to

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-27 Thread Joshua Cohen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/#review150628 --- Ship it! lgtm overall, thanks for iterating! Modulo the below

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-26 Thread Kai Huang
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/#review150428 --- src/main/python/apache/aurora/executor/common/health_checker.py

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-26 Thread Zameer Manji
> On Sept. 23, 2016, 3:18 p.m., Zameer Manji wrote: > > src/main/python/apache/aurora/executor/aurora_executor.py, line 120 > > > > > > This check is brittle to determine if health checking is enabled. > > Please

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-26 Thread Zameer Manji
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/#review150427 --- Ship it! Ship It! - Zameer Manji On Sept. 23, 2016, 11:58

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-23 Thread Kai Huang
> On Sept. 23, 2016, 10:18 p.m., Zameer Manji wrote: > > src/main/python/apache/aurora/executor/aurora_executor.py, line 120 > > > > > > This check is brittle to determine if health checking is enabled. > > Please

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-23 Thread Zameer Manji
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/#review150246 --- I like the additional testing done.

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-23 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/#review150226 --- Ship it! Master (4ead189) is green with this patch.

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-23 Thread Kai Huang
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/ --- (Updated Sept. 23, 2016, 6:58 p.m.) Review request for Aurora, Joshua Cohen,

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-23 Thread Kai Huang
> On Sept. 23, 2016, 4:53 p.m., Joshua Cohen wrote: > > src/main/python/apache/aurora/executor/aurora_executor.py, line 81 > > > > > > I don't think we should expose this simply for the sake of testing, > > besides

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-23 Thread Joshua Cohen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/#review150200 --- src/main/python/apache/aurora/executor/aurora_executor.py (line

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-22 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/#review150117 --- Ship it! Master (4ead189) is green with this patch.

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-22 Thread Kai Huang
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/#review150115 --- src/main/python/apache/aurora/executor/aurora_executor.py (line

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-22 Thread Kai Huang
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/ --- (Updated Sept. 23, 2016, 12:57 a.m.) Review request for Aurora, Joshua Cohen,

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-22 Thread Kai Huang
> On Sept. 21, 2016, 5:34 p.m., Maxim Khutornenko wrote: > > src/main/python/apache/aurora/executor/common/health_checker.py, lines > > 279-283 > > > > > > This logic needs to be refactored to reduce duplication

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-21 Thread Kai Huang
> On Sept. 21, 2016, 5:34 p.m., Maxim Khutornenko wrote: > > src/main/python/apache/aurora/executor/common/health_checker.py, line 149 > > > > > > leftover? will remove it. - Kai

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-21 Thread Kai Huang
> On Sept. 21, 2016, 5:34 p.m., Maxim Khutornenko wrote: > > src/main/python/apache/aurora/executor/aurora_executor.py, lines 184-185 > > > > > > Are there any guarantees the status_manager will always deal with >

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-21 Thread Kai Huang
> On Sept. 21, 2016, 5:34 p.m., Maxim Khutornenko wrote: > > src/main/python/apache/aurora/executor/aurora_executor.py, lines 184-185 > > > > > > Are there any guarantees the status_manager will always deal with >

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-21 Thread Kai Huang
> On Sept. 21, 2016, 5:34 p.m., Maxim Khutornenko wrote: > > src/main/python/apache/aurora/executor/common/health_checker.py, lines > > 162-164 > > > > > > This should be moved into the

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-21 Thread Kai Huang
> On Sept. 21, 2016, 6:25 p.m., Joshua Cohen wrote: > > src/main/python/apache/aurora/executor/common/health_checker.py, line 135 > > > > > > Should this be a call to the `initial_in_progress` function? If so, > >

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-21 Thread Joshua Cohen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/#review149859 --- src/main/python/apache/aurora/executor/aurora_executor.py (lines

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-21 Thread Kai Huang
> On Sept. 21, 2016, 5:34 p.m., Maxim Khutornenko wrote: > > src/main/python/apache/aurora/executor/status_manager.py, line 68 > > > > > > This is exactly the problem I alluded to above. What if there is no > >

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-21 Thread Kai Huang
> On Sept. 21, 2016, 5:34 p.m., Maxim Khutornenko wrote: > > api/src/main/thrift/org/apache/aurora/gen/api.thrift, lines 1218-1220 > > > > > > What's the motivation behind moving these constants into the thrift >

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-21 Thread Kai Huang
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/#review149864 --- src/main/python/apache/aurora/executor/common/health_checker.py

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-21 Thread Maxim Khutornenko
> On Sept. 20, 2016, 12:28 a.m., Kai Huang wrote: > > src/main/python/apache/aurora/executor/common/status_checker.py, line 104 > > > > > > Use TaskState.Value('TASK_RUNNING') here instead of > >

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-21 Thread Maxim Khutornenko
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/#review149846 --- api/src/main/thrift/org/apache/aurora/gen/api.thrift (lines 1218

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-19 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/#review149588 --- Ship it! Master (4c4040f) is green with this patch.

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-19 Thread Kai Huang
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/#review149581 --- src/main/python/apache/aurora/executor/common/status_checker.py

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-19 Thread Kai Huang
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/ --- (Updated Sept. 20, 2016, 12:25 a.m.) Review request for Aurora, Joshua Cohen,

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-13 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/#review148834 --- Master (5069f93) is red with this patch.

Re: Review Request 51876: Modify executor state transition logic to rely on health checks (if enabled)

2016-09-13 Thread Kai Huang
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51876/ --- (Updated Sept. 14, 2016, 12:40 a.m.) Review request for Aurora, Joshua Cohen,