Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-17 Thread Dmitriy Shirchenko 


> On Dec. 16, 2015, 10:14 p.m., Joshua Cohen wrote:
> > src/test/python/apache/aurora/client/test_config.py, line 187
> > 
> >
> > Kill this comment? It seems superfluous.
> 
> Dmitriy Shirchenko wrote:
> I added because I had a hard time finding my tests. I like comments to 
> logically separate tests into sections. If you feel like they should be 
> separated into individual classes, then I can do that as well but comments 
> seemed good enough for tests. Lmk.
> 
> Joshua Cohen wrote:
> I generally prefer to rely on explicitly named tests for this. Adding a 
> comment to delineate a group of tests is vulnerable to someone unknowingly 
> sneaking an unrelated test in the middle of that section. If you feel that 
> there are tests that should be logically grouped, adding them to a class is 
> the way to go.

Deleted (under protest)! I don't want to separate them into classes as I don't 
want to drag out this diff even further and get blocked on comments in tests. :)


> On Dec. 16, 2015, 10:14 p.m., Joshua Cohen wrote:
> > src/test/python/apache/aurora/client/test_config.py, lines 240-243
> > 
> >
> > I'm somewhat uncomfortable with using monkeypatch to accomplish this. 
> > We had generally been trending towards adding injection points for mocks, 
> > rather than monkeypatching globals. How would you feel about adding a 
> > logger arg to `_validate_health_check_config` that just defaults to `log`? 
> > and passing in a mock from this test case?
> 
> Dmitriy Shirchenko wrote:
> I personally use mock.patch decorators myself but I try not to add 
> dependencies as well so I had to figure out how to make this work with 
> pytest. I feel like mock.patch is the way to go but I also feel like we 
> should be consistent and swapping to mock is a separate chunk of work as it 
> may be required affect other tests as well. Like it would be weird to just 
> have one test using mock.
> 
> In any case: is this a nit or you feel like this is a blocker?
> 
> Joshua Cohen wrote:
> We use mock.patch elsewhere throughout the codebase, so adding a 
> dependency on that is not a problem.
> 
> That said, I think using mock.patch is essentially the same thing as 
> using pytest monkeypatch. Perhaps it's due to the fact that this is a python 
> codebase maintained primarily by a bunch of Java developers, but we've tended 
> to prefer injection over patching. The way we accomplish these sort of 
> assertions elsewhere in the client is through the use of the 
> [FakeAuroraCommandContext](https://github.com/apache/aurora/blob/master/src/test/python/apache/aurora/client/cli/util.py#L71)
>  which abstracts away the log statements in a way that makes them easy to 
> mock. However config is at a higher level and `AuroraCommandContext` is not 
> used, but we can still follow a similar pattern with regards to asserting on 
> cli logging.
> 
> So... long story short, this is more than a nit but less than a blocker, 
> hopefully the above makes a compelling case for "injecting" a mock?

If this is not a blocker, then I would prefer to leave this as is.

I am totally onboard with `mock.patch` and you can see test_base.py where I 
used injections all around.


- Dmitriy


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


On Dec. 16, 2015, 10:43 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41428/
> ---
> 
> (Updated Dec. 16, 2015, 10:43 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-1562
> https://issues.apache.org/jira/browse/AURORA-1562
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Refactoring HealthCheckConfig into separate structs
> 
> 
> Diffs
> -
> 
>   docs/configuration-reference.md 07149c7f49371e39b70e8744d60affeffd73c77f 
>   src/main/python/apache/aurora/client/config.py 
> 161c3627db0afa8fdd8afff5abf6a94556f53180 
>   src/main/python/apache/aurora/config/schema/base.py 
> e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
>   src/test/python/apache/aurora/client/test_config.py 
> 8fd112fd8a10120f57324d8efec22a009b93404b 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 8561abc3e7df8803785b70064bb76553d3210399 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
> 37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 
> 
>

Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-17 Thread Bill Farner 


> On Dec. 17, 2015, 12:31 p.m., Bill Farner wrote:
> > Shape of the API LGTM, end-to-end tests pass.

I'm going to commit this.  Maxim - if you have any remaining issues, i'm happy 
to address follow-up comments on Dmitriy's behalf


- Bill


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


On Dec. 17, 2015, 11:21 a.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41428/
> ---
> 
> (Updated Dec. 17, 2015, 11:21 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-1562
> https://issues.apache.org/jira/browse/AURORA-1562
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Refactoring HealthCheckConfig into separate structs
> 
> 
> Diffs
> -
> 
>   docs/configuration-reference.md 07149c7f49371e39b70e8744d60affeffd73c77f 
>   src/main/python/apache/aurora/client/config.py 
> 161c3627db0afa8fdd8afff5abf6a94556f53180 
>   src/main/python/apache/aurora/config/schema/base.py 
> e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
>   src/test/python/apache/aurora/client/test_config.py 
> 8fd112fd8a10120f57324d8efec22a009b93404b 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 8561abc3e7df8803785b70064bb76553d3210399 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
> 37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 
> 
> Diff: https://reviews.apache.org/r/41428/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests.
> Changed end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-17 Thread Dmitriy Shirchenko 


> On Dec. 17, 2015, 1:16 a.m., Joshua Cohen wrote:
> > src/main/python/apache/aurora/executor/common/health_checker.py, line 214
> > 
> >
> > Rename this var too? `_end` was never great, but now it's lost all 
> > relevance ;).

Done.


- Dmitriy


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


On Dec. 16, 2015, 10:43 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41428/
> ---
> 
> (Updated Dec. 16, 2015, 10:43 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-1562
> https://issues.apache.org/jira/browse/AURORA-1562
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Refactoring HealthCheckConfig into separate structs
> 
> 
> Diffs
> -
> 
>   docs/configuration-reference.md 07149c7f49371e39b70e8744d60affeffd73c77f 
>   src/main/python/apache/aurora/client/config.py 
> 161c3627db0afa8fdd8afff5abf6a94556f53180 
>   src/main/python/apache/aurora/config/schema/base.py 
> e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
>   src/test/python/apache/aurora/client/test_config.py 
> 8fd112fd8a10120f57324d8efec22a009b93404b 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 8561abc3e7df8803785b70064bb76553d3210399 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
> 37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 
> 
> Diff: https://reviews.apache.org/r/41428/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests.
> Changed end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-17 Thread Bill Farner 

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

Ship it!


Shape of the API LGTM, end-to-end tests pass.

- Bill Farner


On Dec. 17, 2015, 11:21 a.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41428/
> ---
> 
> (Updated Dec. 17, 2015, 11:21 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-1562
> https://issues.apache.org/jira/browse/AURORA-1562
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Refactoring HealthCheckConfig into separate structs
> 
> 
> Diffs
> -
> 
>   docs/configuration-reference.md 07149c7f49371e39b70e8744d60affeffd73c77f 
>   src/main/python/apache/aurora/client/config.py 
> 161c3627db0afa8fdd8afff5abf6a94556f53180 
>   src/main/python/apache/aurora/config/schema/base.py 
> e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
>   src/test/python/apache/aurora/client/test_config.py 
> 8fd112fd8a10120f57324d8efec22a009b93404b 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 8561abc3e7df8803785b70064bb76553d3210399 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
> 37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 
> 
> Diff: https://reviews.apache.org/r/41428/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests.
> Changed end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-17 Thread Dmitriy Shirchenko 

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

(Updated Dec. 17, 2015, 7:21 p.m.)


Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.


Bugs: AURORA-1562
https://issues.apache.org/jira/browse/AURORA-1562


Repository: aurora


Description
---

Refactoring HealthCheckConfig into separate structs


Diffs (updated)
-

  docs/configuration-reference.md 07149c7f49371e39b70e8744d60affeffd73c77f 
  src/main/python/apache/aurora/client/config.py 
161c3627db0afa8fdd8afff5abf6a94556f53180 
  src/main/python/apache/aurora/config/schema/base.py 
e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
  src/main/python/apache/aurora/executor/common/health_checker.py 
cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
  src/test/python/apache/aurora/client/test_config.py 
8fd112fd8a10120f57324d8efec22a009b93404b 
  src/test/python/apache/aurora/executor/common/test_health_checker.py 
8561abc3e7df8803785b70064bb76553d3210399 
  src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 

Diff: https://reviews.apache.org/r/41428/diff/


Testing
---

Added unit tests.
Changed end to end test.


Thanks,

Dmitriy Shirchenko

Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-16 Thread Aurora ReviewBot 

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

Ship it!


Master (fb8155d) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Dec. 16, 2015, 7:08 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41428/
> ---
> 
> (Updated Dec. 16, 2015, 7:08 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-1562
> https://issues.apache.org/jira/browse/AURORA-1562
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Refactoring HealthCheckConfig into separate structs
> 
> 
> Diffs
> -
> 
>   docs/configuration-reference.md 07149c7f49371e39b70e8744d60affeffd73c77f 
>   src/main/python/apache/aurora/client/config.py 
> 161c3627db0afa8fdd8afff5abf6a94556f53180 
>   src/main/python/apache/aurora/config/schema/base.py 
> e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
>   src/test/python/apache/aurora/client/test_config.py 
> 8fd112fd8a10120f57324d8efec22a009b93404b 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 8561abc3e7df8803785b70064bb76553d3210399 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
> 37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 
> 
> Diff: https://reviews.apache.org/r/41428/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests.
> Changed end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-16 Thread Maxim Khutornenko 


> On Dec. 16, 2015, 4:22 p.m., Joshua Cohen wrote:
> > I'd like to propose an alternate configuration schema for consideration. I 
> > feel like the version presented in this review is redundant and tries to 
> > shoehorn the "endpoint" concept onto a shell health checker where it 
> > doesn't make sense. I think ideally we'd do something like...
> > 
> > health_check_config = HealthCheckConfig(
> > initial_interval_secs = 90,
> > interval_secs = 30,
> > health_checker = ShellHealthChecker(command='grep foo'))
> > 
> > or
> > 
> > health_check_config = HealthCheckConfig(
> > initial_interval_secs = 90,
> > interval_secs = 30,
> > health_checker = HttpHealthChecker(
> > endpoint = '/health',
> > expected_response = '',
> > expected_response_code = '204'))
> > 
> > From what I can see though, pystachio does not support polymorphism along 
> > these lines. So I guess instead we'd have to do something like...
> > 
> > health_check_config = HealthCheckConfig(
> > initial_interval_secs = 90,
> > interval_secs = 30,
> > health_checker = 
> > HealthChecker(shell=ShellHealthChecker(command='grep foo')))
> > 
> > or
> > 
> > health_check_config = HealthCheckConfig(
> > initial_interval_secs = 90,
> > interval_secs = 30,
> > health_checker = HealthChecker(http=HttpHealthChecker(...)))
> > 
> > So I guess what I'm getting at is, instead of calling everything an 
> > `endpoint`, let's call them `health_checker`s?

Just to be clear, what you are proposing is a different naming of the 
fields/objects rather than originally stated alternative schema, is that 
correct?


- Maxim


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


On Dec. 16, 2015, 7:15 a.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41428/
> ---
> 
> (Updated Dec. 16, 2015, 7:15 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1562
> https://issues.apache.org/jira/browse/AURORA-1562
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Refactoring HealthCheckConfig into separate structs
> 
> 
> Diffs
> -
> 
>   docs/configuration-reference.md 07149c7f49371e39b70e8744d60affeffd73c77f 
>   src/main/python/apache/aurora/client/config.py 
> 161c3627db0afa8fdd8afff5abf6a94556f53180 
>   src/main/python/apache/aurora/config/schema/base.py 
> e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
>   src/test/python/apache/aurora/client/test_config.py 
> 8fd112fd8a10120f57324d8efec22a009b93404b 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 8561abc3e7df8803785b70064bb76553d3210399 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
> 37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 
> 
> Diff: https://reviews.apache.org/r/41428/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests.
> Changed end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-16 Thread John Sirois 

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

Ship it!


Master (fb8155d) is green with this patch.
  true

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- John Sirois


On Dec. 16, 2015, 7:15 a.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41428/
> ---
> 
> (Updated Dec. 16, 2015, 7:15 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1562
> https://issues.apache.org/jira/browse/AURORA-1562
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Refactoring HealthCheckConfig into separate structs
> 
> 
> Diffs
> -
> 
>   docs/configuration-reference.md 07149c7f49371e39b70e8744d60affeffd73c77f 
>   src/main/python/apache/aurora/client/config.py 
> 161c3627db0afa8fdd8afff5abf6a94556f53180 
>   src/main/python/apache/aurora/config/schema/base.py 
> e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
>   src/test/python/apache/aurora/client/test_config.py 
> 8fd112fd8a10120f57324d8efec22a009b93404b 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 8561abc3e7df8803785b70064bb76553d3210399 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
> 37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 
> 
> Diff: https://reviews.apache.org/r/41428/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests.
> Changed end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-16 Thread Bill Farner 


> On Dec. 16, 2015, 8:22 a.m., Joshua Cohen wrote:
> > I'd like to propose an alternate configuration schema for consideration. I 
> > feel like the version presented in this review is redundant and tries to 
> > shoehorn the "endpoint" concept onto a shell health checker where it 
> > doesn't make sense. I think ideally we'd do something like...
> > 
> > health_check_config = HealthCheckConfig(
> > initial_interval_secs = 90,
> > interval_secs = 30,
> > health_checker = ShellHealthChecker(command='grep foo'))
> > 
> > or
> > 
> > health_check_config = HealthCheckConfig(
> > initial_interval_secs = 90,
> > interval_secs = 30,
> > health_checker = HttpHealthChecker(
> > endpoint = '/health',
> > expected_response = '',
> > expected_response_code = '204'))
> > 
> > From what I can see though, pystachio does not support polymorphism along 
> > these lines. So I guess instead we'd have to do something like...
> > 
> > health_check_config = HealthCheckConfig(
> > initial_interval_secs = 90,
> > interval_secs = 30,
> > health_checker = 
> > HealthChecker(shell=ShellHealthChecker(command='grep foo')))
> > 
> > or
> > 
> > health_check_config = HealthCheckConfig(
> > initial_interval_secs = 90,
> > interval_secs = 30,
> > health_checker = HealthChecker(http=HttpHealthChecker(...)))
> > 
> > So I guess what I'm getting at is, instead of calling everything an 
> > `endpoint`, let's call them `health_checker`s?
> 
> Maxim Khutornenko wrote:
> Just to be clear, what you are proposing is a different naming of the 
> fields/objects rather than originally stated alternative schema, is that 
> correct?
> 
> Joshua Cohen wrote:
> I don't think the alternative schema is *possible*. If it were, I think 
> that would be ideal, but given that, yes, just renaming the fields/objects 
> will suffice.

+1 to the renames Joshua proposed.


- Bill


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


On Dec. 15, 2015, 11:15 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41428/
> ---
> 
> (Updated Dec. 15, 2015, 11:15 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1562
> https://issues.apache.org/jira/browse/AURORA-1562
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Refactoring HealthCheckConfig into separate structs
> 
> 
> Diffs
> -
> 
>   docs/configuration-reference.md 07149c7f49371e39b70e8744d60affeffd73c77f 
>   src/main/python/apache/aurora/client/config.py 
> 161c3627db0afa8fdd8afff5abf6a94556f53180 
>   src/main/python/apache/aurora/config/schema/base.py 
> e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
>   src/test/python/apache/aurora/client/test_config.py 
> 8fd112fd8a10120f57324d8efec22a009b93404b 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 8561abc3e7df8803785b70064bb76553d3210399 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
> 37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 
> 
> Diff: https://reviews.apache.org/r/41428/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests.
> Changed end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-16 Thread Zameer Manji 

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


I won't be able to give a review in a reasonable period of time. Please swap me 
out for Joshua.

- Zameer Manji


On Dec. 15, 2015, 11:15 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41428/
> ---
> 
> (Updated Dec. 15, 2015, 11:15 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1562
> https://issues.apache.org/jira/browse/AURORA-1562
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Refactoring HealthCheckConfig into separate structs
> 
> 
> Diffs
> -
> 
>   docs/configuration-reference.md 07149c7f49371e39b70e8744d60affeffd73c77f 
>   src/main/python/apache/aurora/client/config.py 
> 161c3627db0afa8fdd8afff5abf6a94556f53180 
>   src/main/python/apache/aurora/config/schema/base.py 
> e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
>   src/test/python/apache/aurora/client/test_config.py 
> 8fd112fd8a10120f57324d8efec22a009b93404b 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 8561abc3e7df8803785b70064bb76553d3210399 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
> 37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 
> 
> Diff: https://reviews.apache.org/r/41428/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests.
> Changed end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-16 Thread Dmitriy Shirchenko 

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

(Updated Dec. 16, 2015, 7:08 p.m.)


Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.


Bugs: AURORA-1562
https://issues.apache.org/jira/browse/AURORA-1562


Repository: aurora


Description
---

Refactoring HealthCheckConfig into separate structs


Diffs (updated)
-

  docs/configuration-reference.md 07149c7f49371e39b70e8744d60affeffd73c77f 
  src/main/python/apache/aurora/client/config.py 
161c3627db0afa8fdd8afff5abf6a94556f53180 
  src/main/python/apache/aurora/config/schema/base.py 
e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
  src/main/python/apache/aurora/executor/common/health_checker.py 
cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
  src/test/python/apache/aurora/client/test_config.py 
8fd112fd8a10120f57324d8efec22a009b93404b 
  src/test/python/apache/aurora/executor/common/test_health_checker.py 
8561abc3e7df8803785b70064bb76553d3210399 
  src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 

Diff: https://reviews.apache.org/r/41428/diff/


Testing
---

Added unit tests.
Changed end to end test.


Thanks,

Dmitriy Shirchenko

Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-16 Thread Aurora ReviewBot 

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

Ship it!


Master (fb8155d) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Dec. 16, 2015, 7:15 a.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41428/
> ---
> 
> (Updated Dec. 16, 2015, 7:15 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1562
> https://issues.apache.org/jira/browse/AURORA-1562
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Refactoring HealthCheckConfig into separate structs
> 
> 
> Diffs
> -
> 
>   docs/configuration-reference.md 07149c7f49371e39b70e8744d60affeffd73c77f 
>   src/main/python/apache/aurora/client/config.py 
> 161c3627db0afa8fdd8afff5abf6a94556f53180 
>   src/main/python/apache/aurora/config/schema/base.py 
> e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
>   src/test/python/apache/aurora/client/test_config.py 
> 8fd112fd8a10120f57324d8efec22a009b93404b 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 8561abc3e7df8803785b70064bb76553d3210399 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
> 37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 
> 
> Diff: https://reviews.apache.org/r/41428/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests.
> Changed end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-16 Thread Joshua Cohen 


> On Dec. 16, 2015, 4:22 p.m., Joshua Cohen wrote:
> > I'd like to propose an alternate configuration schema for consideration. I 
> > feel like the version presented in this review is redundant and tries to 
> > shoehorn the "endpoint" concept onto a shell health checker where it 
> > doesn't make sense. I think ideally we'd do something like...
> > 
> > health_check_config = HealthCheckConfig(
> > initial_interval_secs = 90,
> > interval_secs = 30,
> > health_checker = ShellHealthChecker(command='grep foo'))
> > 
> > or
> > 
> > health_check_config = HealthCheckConfig(
> > initial_interval_secs = 90,
> > interval_secs = 30,
> > health_checker = HttpHealthChecker(
> > endpoint = '/health',
> > expected_response = '',
> > expected_response_code = '204'))
> > 
> > From what I can see though, pystachio does not support polymorphism along 
> > these lines. So I guess instead we'd have to do something like...
> > 
> > health_check_config = HealthCheckConfig(
> > initial_interval_secs = 90,
> > interval_secs = 30,
> > health_checker = 
> > HealthChecker(shell=ShellHealthChecker(command='grep foo')))
> > 
> > or
> > 
> > health_check_config = HealthCheckConfig(
> > initial_interval_secs = 90,
> > interval_secs = 30,
> > health_checker = HealthChecker(http=HttpHealthChecker(...)))
> > 
> > So I guess what I'm getting at is, instead of calling everything an 
> > `endpoint`, let's call them `health_checker`s?
> 
> Maxim Khutornenko wrote:
> Just to be clear, what you are proposing is a different naming of the 
> fields/objects rather than originally stated alternative schema, is that 
> correct?

I don't think the alternative schema is *possible*. If it were, I think that 
would be ideal, but given that, yes, just renaming the fields/objects will 
suffice.


- Joshua


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


On Dec. 16, 2015, 7:15 a.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41428/
> ---
> 
> (Updated Dec. 16, 2015, 7:15 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1562
> https://issues.apache.org/jira/browse/AURORA-1562
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Refactoring HealthCheckConfig into separate structs
> 
> 
> Diffs
> -
> 
>   docs/configuration-reference.md 07149c7f49371e39b70e8744d60affeffd73c77f 
>   src/main/python/apache/aurora/client/config.py 
> 161c3627db0afa8fdd8afff5abf6a94556f53180 
>   src/main/python/apache/aurora/config/schema/base.py 
> e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
>   src/test/python/apache/aurora/client/test_config.py 
> 8fd112fd8a10120f57324d8efec22a009b93404b 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 8561abc3e7df8803785b70064bb76553d3210399 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
> 37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 
> 
> Diff: https://reviews.apache.org/r/41428/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests.
> Changed end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-16 Thread Dmitriy Shirchenko 


> On Dec. 16, 2015, 4:22 p.m., Joshua Cohen wrote:
> > I'd like to propose an alternate configuration schema for consideration. I 
> > feel like the version presented in this review is redundant and tries to 
> > shoehorn the "endpoint" concept onto a shell health checker where it 
> > doesn't make sense. I think ideally we'd do something like...
> > 
> > health_check_config = HealthCheckConfig(
> > initial_interval_secs = 90,
> > interval_secs = 30,
> > health_checker = ShellHealthChecker(command='grep foo'))
> > 
> > or
> > 
> > health_check_config = HealthCheckConfig(
> > initial_interval_secs = 90,
> > interval_secs = 30,
> > health_checker = HttpHealthChecker(
> > endpoint = '/health',
> > expected_response = '',
> > expected_response_code = '204'))
> > 
> > From what I can see though, pystachio does not support polymorphism along 
> > these lines. So I guess instead we'd have to do something like...
> > 
> > health_check_config = HealthCheckConfig(
> > initial_interval_secs = 90,
> > interval_secs = 30,
> > health_checker = 
> > HealthChecker(shell=ShellHealthChecker(command='grep foo')))
> > 
> > or
> > 
> > health_check_config = HealthCheckConfig(
> > initial_interval_secs = 90,
> > interval_secs = 30,
> > health_checker = HealthChecker(http=HttpHealthChecker(...)))
> > 
> > So I guess what I'm getting at is, instead of calling everything an 
> > `endpoint`, let's call them `health_checker`s?
> 
> Maxim Khutornenko wrote:
> Just to be clear, what you are proposing is a different naming of the 
> fields/objects rather than originally stated alternative schema, is that 
> correct?
> 
> Joshua Cohen wrote:
> I don't think the alternative schema is *possible*. If it were, I think 
> that would be ideal, but given that, yes, just renaming the fields/objects 
> will suffice.
> 
> Bill Farner wrote:
> +1 to the renames Joshua proposed.

Done.


- Dmitriy


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


On Dec. 16, 2015, 7:07 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41428/
> ---
> 
> (Updated Dec. 16, 2015, 7:07 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-1562
> https://issues.apache.org/jira/browse/AURORA-1562
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Refactoring HealthCheckConfig into separate structs
> 
> 
> Diffs
> -
> 
>   docs/configuration-reference.md 07149c7f49371e39b70e8744d60affeffd73c77f 
>   src/main/python/apache/aurora/client/config.py 
> 161c3627db0afa8fdd8afff5abf6a94556f53180 
>   src/main/python/apache/aurora/config/schema/base.py 
> e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
>   src/test/python/apache/aurora/client/test_config.py 
> 8fd112fd8a10120f57324d8efec22a009b93404b 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 8561abc3e7df8803785b70064bb76553d3210399 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
> 37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 
> 
> Diff: https://reviews.apache.org/r/41428/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests.
> Changed end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-16 Thread Joshua Cohen 

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



src/main/python/apache/aurora/client/config.py (line 103)


Try to avoid abbreviations in variable names. Just call this 
`shell_health_checker`?



src/main/python/apache/aurora/client/config.py (lines 103 - 105)


Nit, could probably inline all of this:

if not health_check_end_config.get(SHELL_HEALTH_CHECK, 
{}).get('shell_command'):
  die(...)



src/main/python/apache/aurora/config/schema/base.py (line 59)


We should also rename `endpoint_config` to `health_checker`.

And probably rename the above `DefaultEndpointConfig` to 
`DefaultHealthChecker`



src/test/python/apache/aurora/client/test_config.py (line 187)


Kill this comment? It seems superfluous.



src/test/python/apache/aurora/client/test_config.py (lines 236 - 239)


I'm somewhat uncomfortable with using monkeypatch to accomplish this. We 
had generally been trending towards adding injection points for mocks, rather 
than monkeypatching globals. How would you feel about adding a logger arg to 
`_validate_health_check_config` that just defaults to `log`? and passing in a 
mock from this test case?



src/test/python/apache/aurora/client/test_config.py (line 252)


This test is not doing what it claims to do. It's verifying that we raise 
an AttributeError if an unexpected value is passed in (foo), not that things 
fail if no command is specified.

Pystachio does support type checking for required fields, but it has to be 
explicitly invoked (i.e. you can construct a `ShellHealthChecker` without 
passing in a command, but if you were to call `check()` on it, it would return 
with a TypeCheck error. I'm not super familiar with this part of the codebase, 
but it looks like maybe we do this when we load the config (though I'm not 100% 
sure).

I'd try loading a config as the above tests do, with `health_checker = 
ShellHealthChecker()` and see what happens, hopefully that will fail with an 
invalid config due to missing command line, and we can assert on that failure?



src/test/python/apache/aurora/client/test_config.py (line 272)


Drop this comment as well.


- Joshua Cohen


On Dec. 16, 2015, 7:08 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41428/
> ---
> 
> (Updated Dec. 16, 2015, 7:08 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-1562
> https://issues.apache.org/jira/browse/AURORA-1562
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Refactoring HealthCheckConfig into separate structs
> 
> 
> Diffs
> -
> 
>   docs/configuration-reference.md 07149c7f49371e39b70e8744d60affeffd73c77f 
>   src/main/python/apache/aurora/client/config.py 
> 161c3627db0afa8fdd8afff5abf6a94556f53180 
>   src/main/python/apache/aurora/config/schema/base.py 
> e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
>   src/test/python/apache/aurora/client/test_config.py 
> 8fd112fd8a10120f57324d8efec22a009b93404b 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 8561abc3e7df8803785b70064bb76553d3210399 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
> 37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 
> 
> Diff: https://reviews.apache.org/r/41428/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests.
> Changed end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-16 Thread Dmitriy Shirchenko 

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

(Updated Dec. 16, 2015, 10:43 p.m.)


Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.


Bugs: AURORA-1562
https://issues.apache.org/jira/browse/AURORA-1562


Repository: aurora


Description
---

Refactoring HealthCheckConfig into separate structs


Diffs (updated)
-

  docs/configuration-reference.md 07149c7f49371e39b70e8744d60affeffd73c77f 
  src/main/python/apache/aurora/client/config.py 
161c3627db0afa8fdd8afff5abf6a94556f53180 
  src/main/python/apache/aurora/config/schema/base.py 
e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
  src/main/python/apache/aurora/executor/common/health_checker.py 
cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
  src/test/python/apache/aurora/client/test_config.py 
8fd112fd8a10120f57324d8efec22a009b93404b 
  src/test/python/apache/aurora/executor/common/test_health_checker.py 
8561abc3e7df8803785b70064bb76553d3210399 
  src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 

Diff: https://reviews.apache.org/r/41428/diff/


Testing
---

Added unit tests.
Changed end to end test.


Thanks,

Dmitriy Shirchenko

Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-16 Thread Dmitriy Shirchenko 


> On Dec. 16, 2015, 10:14 p.m., Joshua Cohen wrote:
> > src/main/python/apache/aurora/client/config.py, lines 111-113
> > 
> >
> > Nit, could probably inline all of this:
> > 
> > if not health_check_end_config.get(SHELL_HEALTH_CHECK, 
> > {}).get('shell_command'):
> >   die(...)

I personally prefer not to chain more than 2 operations on one line as it makes 
it harder for me to step through variables when debugging. So, if I may, I will 
respectfully decline this suggestion.


> On Dec. 16, 2015, 10:14 p.m., Joshua Cohen wrote:
> > src/test/python/apache/aurora/client/test_config.py, lines 240-243
> > 
> >
> > I'm somewhat uncomfortable with using monkeypatch to accomplish this. 
> > We had generally been trending towards adding injection points for mocks, 
> > rather than monkeypatching globals. How would you feel about adding a 
> > logger arg to `_validate_health_check_config` that just defaults to `log`? 
> > and passing in a mock from this test case?

I personally use mock.patch decorators myself but I try not to add dependencies 
as well so I had to figure out how to make this work with pytest. I feel like 
mock.patch is the way to go but I also feel like we should be consistent and 
swapping to mock is a separate chunk of work as it may be required affect other 
tests as well. Like it would be weird to just have one test using mock.

In any case: is this a nit or you feel like this is a blocker?


> On Dec. 16, 2015, 10:14 p.m., Joshua Cohen wrote:
> > src/test/python/apache/aurora/client/test_config.py, line 187
> > 
> >
> > Kill this comment? It seems superfluous.

I added because I had a hard time finding my tests. I like comments to 
logically separate tests into sections. If you feel like they should be 
separated into individual classes, then I can do that as well but comments 
seemed good enough for tests. Lmk.


> On Dec. 16, 2015, 10:14 p.m., Joshua Cohen wrote:
> > src/test/python/apache/aurora/client/test_config.py, line 276
> > 
> >
> > Drop this comment as well.

Same response as above. ^


> On Dec. 16, 2015, 10:14 p.m., Joshua Cohen wrote:
> > src/main/python/apache/aurora/client/config.py, line 111
> > 
> >
> > Try to avoid abbreviations in variable names. Just call this 
> > `shell_health_checker`?

Done.


> On Dec. 16, 2015, 10:14 p.m., Joshua Cohen wrote:
> > src/main/python/apache/aurora/config/schema/base.py, line 59
> > 
> >
> > We should also rename `endpoint_config` to `health_checker`.
> > 
> > And probably rename the above `DefaultEndpointConfig` to 
> > `DefaultHealthChecker`

Done.


> On Dec. 16, 2015, 10:14 p.m., Joshua Cohen wrote:
> > src/test/python/apache/aurora/client/test_config.py, line 256
> > 
> >
> > This test is not doing what it claims to do. It's verifying that we 
> > raise an AttributeError if an unexpected value is passed in (foo), not that 
> > things fail if no command is specified.
> > 
> > Pystachio does support type checking for required fields, but it has to 
> > be explicitly invoked (i.e. you can construct a `ShellHealthChecker` 
> > without passing in a command, but if you were to call `check()` on it, it 
> > would return with a TypeCheck error. I'm not super familiar with this part 
> > of the codebase, but it looks like maybe we do this when we load the config 
> > (though I'm not 100% sure).
> > 
> > I'd try loading a config as the above tests do, with `health_checker = 
> > ShellHealthChecker()` and see what happens, hopefully that will fail with 
> > an invalid config due to missing command line, and we can assert on that 
> > failure?

Done.


- Dmitriy


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


On Dec. 16, 2015, 7:08 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41428/
> ---
> 
> (Updated Dec. 16, 2015, 7:08 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-1562
> https://issues.apache.org/jira/browse/AURORA-1562
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Refactoring

Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-16 Thread Joshua Cohen 


> On Dec. 16, 2015, 10:14 p.m., Joshua Cohen wrote:
> > src/test/python/apache/aurora/client/test_config.py, line 187
> > 
> >
> > Kill this comment? It seems superfluous.
> 
> Dmitriy Shirchenko wrote:
> I added because I had a hard time finding my tests. I like comments to 
> logically separate tests into sections. If you feel like they should be 
> separated into individual classes, then I can do that as well but comments 
> seemed good enough for tests. Lmk.

I generally prefer to rely on explicitly named tests for this. Adding a comment 
to delineate a group of tests is vulnerable to someone unknowingly sneaking an 
unrelated test in the middle of that section. If you feel that there are tests 
that should be logically grouped, adding them to a class is the way to go.


> On Dec. 16, 2015, 10:14 p.m., Joshua Cohen wrote:
> > src/test/python/apache/aurora/client/test_config.py, lines 240-243
> > 
> >
> > I'm somewhat uncomfortable with using monkeypatch to accomplish this. 
> > We had generally been trending towards adding injection points for mocks, 
> > rather than monkeypatching globals. How would you feel about adding a 
> > logger arg to `_validate_health_check_config` that just defaults to `log`? 
> > and passing in a mock from this test case?
> 
> Dmitriy Shirchenko wrote:
> I personally use mock.patch decorators myself but I try not to add 
> dependencies as well so I had to figure out how to make this work with 
> pytest. I feel like mock.patch is the way to go but I also feel like we 
> should be consistent and swapping to mock is a separate chunk of work as it 
> may be required affect other tests as well. Like it would be weird to just 
> have one test using mock.
> 
> In any case: is this a nit or you feel like this is a blocker?

We use mock.patch elsewhere throughout the codebase, so adding a dependency on 
that is not a problem.

That said, I think using mock.patch is essentially the same thing as using 
pytest monkeypatch. Perhaps it's due to the fact that this is a python codebase 
maintained primarily by a bunch of Java developers, but we've tended to prefer 
injection over patching. The way we accomplish these sort of assertions 
elsewhere in the client is through the use of the 
[FakeAuroraCommandContext](https://github.com/apache/aurora/blob/master/src/test/python/apache/aurora/client/cli/util.py#L71)
 which abstracts away the log statements in a way that makes them easy to mock. 
However config is at a higher level and `AuroraCommandContext` is not used, but 
we can still follow a similar pattern with regards to asserting on cli logging.

So... long story short, this is more than a nit but less than a blocker, 
hopefully the above makes a compelling case for "injecting" a mock?


- Joshua


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


On Dec. 16, 2015, 10:43 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41428/
> ---
> 
> (Updated Dec. 16, 2015, 10:43 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-1562
> https://issues.apache.org/jira/browse/AURORA-1562
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Refactoring HealthCheckConfig into separate structs
> 
> 
> Diffs
> -
> 
>   docs/configuration-reference.md 07149c7f49371e39b70e8744d60affeffd73c77f 
>   src/main/python/apache/aurora/client/config.py 
> 161c3627db0afa8fdd8afff5abf6a94556f53180 
>   src/main/python/apache/aurora/config/schema/base.py 
> e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
>   src/test/python/apache/aurora/client/test_config.py 
> 8fd112fd8a10120f57324d8efec22a009b93404b 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 8561abc3e7df8803785b70064bb76553d3210399 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
> 37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 
> 
> Diff: https://reviews.apache.org/r/41428/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests.
> Changed end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-16 Thread Joshua Cohen 

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



src/main/python/apache/aurora/executor/common/health_checker.py (line 214)


Rename this var too? `_end` was never great, but now it's lost all 
relevance ;).


- Joshua Cohen


On Dec. 16, 2015, 10:43 p.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41428/
> ---
> 
> (Updated Dec. 16, 2015, 10:43 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.
> 
> 
> Bugs: AURORA-1562
> https://issues.apache.org/jira/browse/AURORA-1562
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Refactoring HealthCheckConfig into separate structs
> 
> 
> Diffs
> -
> 
>   docs/configuration-reference.md 07149c7f49371e39b70e8744d60affeffd73c77f 
>   src/main/python/apache/aurora/client/config.py 
> 161c3627db0afa8fdd8afff5abf6a94556f53180 
>   src/main/python/apache/aurora/config/schema/base.py 
> e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
>   src/test/python/apache/aurora/client/test_config.py 
> 8fd112fd8a10120f57324d8efec22a009b93404b 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 8561abc3e7df8803785b70064bb76553d3210399 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
> 37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 
> 
> Diff: https://reviews.apache.org/r/41428/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests.
> Changed end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-16 Thread Joshua Cohen 

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


I'd like to propose an alternate configuration schema for consideration. I feel 
like the version presented in this review is redundant and tries to shoehorn 
the "endpoint" concept onto a shell health checker where it doesn't make sense. 
I think ideally we'd do something like...

health_check_config = HealthCheckConfig(
initial_interval_secs = 90,
interval_secs = 30,
health_checker = ShellHealthChecker(command='grep foo'))

or

health_check_config = HealthCheckConfig(
initial_interval_secs = 90,
interval_secs = 30,
health_checker = HttpHealthChecker(
endpoint = '/health',
expected_response = '',
expected_response_code = '204'))

>From what I can see though, pystachio does not support polymorphism along 
>these lines. So I guess instead we'd have to do something like...

health_check_config = HealthCheckConfig(
initial_interval_secs = 90,
interval_secs = 30,
health_checker = HealthChecker(shell=ShellHealthChecker(command='grep 
foo')))

or

health_check_config = HealthCheckConfig(
initial_interval_secs = 90,
interval_secs = 30,
health_checker = HealthChecker(http=HttpHealthChecker(...)))

So I guess what I'm getting at is, instead of calling everything an `endpoint`, 
let's call them `health_checker`s?

- Joshua Cohen


On Dec. 16, 2015, 7:15 a.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41428/
> ---
> 
> (Updated Dec. 16, 2015, 7:15 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1562
> https://issues.apache.org/jira/browse/AURORA-1562
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Refactoring HealthCheckConfig into separate structs
> 
> 
> Diffs
> -
> 
>   docs/configuration-reference.md 07149c7f49371e39b70e8744d60affeffd73c77f 
>   src/main/python/apache/aurora/client/config.py 
> 161c3627db0afa8fdd8afff5abf6a94556f53180 
>   src/main/python/apache/aurora/config/schema/base.py 
> e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
>   src/test/python/apache/aurora/client/test_config.py 
> 8fd112fd8a10120f57324d8efec22a009b93404b 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 8561abc3e7df8803785b70064bb76553d3210399 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
> 37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 
> 
> Diff: https://reviews.apache.org/r/41428/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests.
> Changed end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-15 Thread Dmitriy Shirchenko 

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

Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji.


Bugs: AURORA-1562
https://issues.apache.org/jira/browse/AURORA-1562


Repository: aurora


Description
---

Refactoring HealthCheckConfig into separate structs


Diffs
-

  src/main/python/apache/aurora/client/config.py 
161c3627db0afa8fdd8afff5abf6a94556f53180 
  src/main/python/apache/aurora/config/schema/base.py 
e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
  src/main/python/apache/aurora/executor/common/health_checker.py 
cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
  src/test/python/apache/aurora/client/test_config.py 
8fd112fd8a10120f57324d8efec22a009b93404b 
  src/test/python/apache/aurora/executor/common/test_health_checker.py 
8561abc3e7df8803785b70064bb76553d3210399 
  src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 

Diff: https://reviews.apache.org/r/41428/diff/


Testing
---

Added unit tests.


Thanks,

Dmitriy Shirchenko

Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-15 Thread Maxim Khutornenko 

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


Looks good overall! A few comments below.


src/main/python/apache/aurora/client/config.py (line 82)


"...are deprecated and will be removed in next release."

The last sentence needs some rephrasing as well.



src/main/python/apache/aurora/client/config.py (lines 95 - 96)


Unfinished comments?



src/main/python/apache/aurora/client/config.py (line 97)


s/to_deprecate/deprecated



src/main/python/apache/aurora/config/schema/base.py (line 39)


Please, document new fields in configuration-reference.md and mark old ones 
as deprecated.



src/main/python/apache/aurora/executor/common/health_checker.py (line 234)


Please, create removal ticket, link it to AURORA-1559 and mention it here.



src/test/python/apache/aurora/client/test_config.py (line 217)


tabbing is off (here and below)? should be 4 spaces for continuation.



src/test/python/apache/aurora/client/test_config.py (line 241)


This should be HTTP_DEPRECATION_WARNING from config.py.


- Maxim Khutornenko


On Dec. 16, 2015, 2:13 a.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41428/
> ---
> 
> (Updated Dec. 16, 2015, 2:13 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1562
> https://issues.apache.org/jira/browse/AURORA-1562
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Refactoring HealthCheckConfig into separate structs
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/config.py 
> 161c3627db0afa8fdd8afff5abf6a94556f53180 
>   src/main/python/apache/aurora/config/schema/base.py 
> e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
>   src/test/python/apache/aurora/client/test_config.py 
> 8fd112fd8a10120f57324d8efec22a009b93404b 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 8561abc3e7df8803785b70064bb76553d3210399 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
> 37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 
> 
> Diff: https://reviews.apache.org/r/41428/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-15 Thread Aurora ReviewBot 

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


Master (fb8155d) is red with this patch.
  ./build-support/jenkins/build.sh

 from apache.aurora.config.schema.base import (
-  HealthCheckConfig,
-  HealthEndpointConfig,
-  HttpEndpointConfig,
-  ShellEndpointConfig
+HealthCheckConfig,
+HealthEndpointConfig,
+HttpEndpointConfig,
+ShellEndpointConfig
 )
 from apache.aurora.executor.common.health_checker import (
 HealthChecker,
ERROR: 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/test_config.py
 Imports are incorrectly sorted.
--- 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/test_config.py:before
  2015-12-16 01:09:02.879409
+++ 
/home/jenkins/jenkins-slave/workspace/AuroraBot/src/test/python/apache/aurora/client/test_config.py:after
   2015-12-16 01:15:34.300949
@@ -24,14 +24,16 @@
 from apache.aurora.config import AuroraConfig
 from apache.aurora.config.loader import AuroraConfigLoader
 from apache.aurora.config.schema.base import (
-  MB,
-  Announcer,
-  HealthCheckConfig,
-  Job,
-  Resources,
-  Task,
-  UpdateConfig,
-  HealthEndpointConfig, ShellEndpointConfig)
+MB,
+Announcer,
+HealthCheckConfig,
+HealthEndpointConfig,
+Job,
+Resources,
+ShellEndpointConfig,
+Task,
+UpdateConfig
+)
 
 MESOS_CONFIG_BASE = """
 HELLO_WORLD = Job(


I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Dec. 16, 2015, 12:55 a.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41428/
> ---
> 
> (Updated Dec. 16, 2015, 12:55 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1562
> https://issues.apache.org/jira/browse/AURORA-1562
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Refactoring HealthCheckConfig into separate structs
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/config.py 
> 161c3627db0afa8fdd8afff5abf6a94556f53180 
>   src/main/python/apache/aurora/config/schema/base.py 
> e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
>   src/test/python/apache/aurora/client/test_config.py 
> 8fd112fd8a10120f57324d8efec22a009b93404b 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 8561abc3e7df8803785b70064bb76553d3210399 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
> 37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 
> 
> Diff: https://reviews.apache.org/r/41428/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-15 Thread Aurora ReviewBot 

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

Ship it!


Master (fb8155d) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Dec. 16, 2015, 2:13 a.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41428/
> ---
> 
> (Updated Dec. 16, 2015, 2:13 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1562
> https://issues.apache.org/jira/browse/AURORA-1562
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Refactoring HealthCheckConfig into separate structs
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/config.py 
> 161c3627db0afa8fdd8afff5abf6a94556f53180 
>   src/main/python/apache/aurora/config/schema/base.py 
> e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
>   src/test/python/apache/aurora/client/test_config.py 
> 8fd112fd8a10120f57324d8efec22a009b93404b 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 8561abc3e7df8803785b70064bb76553d3210399 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
> 37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 
> 
> Diff: https://reviews.apache.org/r/41428/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>

Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-15 Thread Dmitriy Shirchenko 

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

(Updated Dec. 16, 2015, 2:13 a.m.)


Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji.


Bugs: AURORA-1562
https://issues.apache.org/jira/browse/AURORA-1562


Repository: aurora


Description
---

Refactoring HealthCheckConfig into separate structs


Diffs (updated)
-

  src/main/python/apache/aurora/client/config.py 
161c3627db0afa8fdd8afff5abf6a94556f53180 
  src/main/python/apache/aurora/config/schema/base.py 
e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
  src/main/python/apache/aurora/executor/common/health_checker.py 
cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
  src/test/python/apache/aurora/client/test_config.py 
8fd112fd8a10120f57324d8efec22a009b93404b 
  src/test/python/apache/aurora/executor/common/test_health_checker.py 
8561abc3e7df8803785b70064bb76553d3210399 
  src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 

Diff: https://reviews.apache.org/r/41428/diff/


Testing
---

Added unit tests.


Thanks,

Dmitriy Shirchenko

Re: Review Request 41428: Refactoring HealthCheckConfig into separate structs

2015-12-15 Thread Aurora ReviewBot 

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

Ship it!


Master (fb8155d) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Dec. 16, 2015, 7:15 a.m., Dmitriy Shirchenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41428/
> ---
> 
> (Updated Dec. 16, 2015, 7:15 a.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1562
> https://issues.apache.org/jira/browse/AURORA-1562
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Refactoring HealthCheckConfig into separate structs
> 
> 
> Diffs
> -
> 
>   docs/configuration-reference.md 07149c7f49371e39b70e8744d60affeffd73c77f 
>   src/main/python/apache/aurora/client/config.py 
> 161c3627db0afa8fdd8afff5abf6a94556f53180 
>   src/main/python/apache/aurora/config/schema/base.py 
> e752482b95ae6377d0cf12cf0ecf0fa60e2a5d46 
>   src/main/python/apache/aurora/executor/common/health_checker.py 
> cba4e8c879fae2b9f56a36bf8fa4b702163667d1 
>   src/test/python/apache/aurora/client/test_config.py 
> 8fd112fd8a10120f57324d8efec22a009b93404b 
>   src/test/python/apache/aurora/executor/common/test_health_checker.py 
> 8561abc3e7df8803785b70064bb76553d3210399 
>   src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora 
> 37f2e9c6ebc4a4bbd6089e821420e5a02179c6b9 
> 
> Diff: https://reviews.apache.org/r/41428/diff/
> 
> 
> Testing
> ---
> 
> Added unit tests.
> Changed end to end test.
> 
> 
> Thanks,
> 
> Dmitriy Shirchenko
> 
>