You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Dmitriy Shirchenko <ca...@gmail.com> on 2015/12/16 01:55:10 UTC
Review Request 41428: Refactoring HealthCheckConfig into separate
structs
-----------------------------------------------------------
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
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
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
Posted by John Sirois <jo...@gmail.com>.
-----------------------------------------------------------
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
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
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
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 41428: Refactoring HealthCheckConfig into separate
structs
Posted by Dmitriy Shirchenko <ca...@gmail.com>.
> On Dec. 16, 2015, 6:45 p.m., Zameer Manji wrote:
> > I won't be able to give a review in a reasonable period of time. Please swap me out for Joshua.
Done.
- Dmitriy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41428/#review110716
-----------------------------------------------------------
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
Posted by Zameer Manji <zm...@apache.org>.
-----------------------------------------------------------
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
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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
Posted by Dmitriy Shirchenko <ca...@gmail.com>.
> 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
Posted by Bill Farner <wf...@apache.org>.
> 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
Posted by Joshua Cohen <jc...@apache.org>.
> 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
Posted by Joshua Cohen <jc...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 41428: Refactoring HealthCheckConfig into separate
structs
Posted by Dmitriy Shirchenko <ca...@gmail.com>.
-----------------------------------------------------------
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 (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 (updated)
-------
Added unit tests.
Changed end to end test.
Thanks,
Dmitriy Shirchenko
Re: Review Request 41428: Refactoring HealthCheckConfig into separate
structs
Posted by Dmitriy Shirchenko <ca...@gmail.com>.
> On Dec. 16, 2015, 4:40 a.m., Maxim Khutornenko wrote:
> > src/main/python/apache/aurora/client/config.py, line 90
> > <https://reviews.apache.org/r/41428/diff/2/?file=1165615#file1165615line90>
> >
> > "...are deprecated and will be removed in next release."
> >
> > The last sentence needs some rephrasing as well.
Done.
> On Dec. 16, 2015, 4:40 a.m., Maxim Khutornenko wrote:
> > src/main/python/apache/aurora/client/config.py, lines 103-104
> > <https://reviews.apache.org/r/41428/diff/2/?file=1165615#file1165615line103>
> >
> > Unfinished comments?
Done.
> On Dec. 16, 2015, 4:40 a.m., Maxim Khutornenko wrote:
> > src/main/python/apache/aurora/client/config.py, line 105
> > <https://reviews.apache.org/r/41428/diff/2/?file=1165615#file1165615line105>
> >
> > s/to_deprecate/deprecated
Done.
> On Dec. 16, 2015, 4:40 a.m., Maxim Khutornenko wrote:
> > src/test/python/apache/aurora/client/test_config.py, line 245
> > <https://reviews.apache.org/r/41428/diff/2/?file=1165618#file1165618line245>
> >
> > This should be HTTP_DEPRECATION_WARNING from config.py.
Done.
> On Dec. 16, 2015, 4:40 a.m., Maxim Khutornenko wrote:
> > src/test/python/apache/aurora/client/test_config.py, line 218
> > <https://reviews.apache.org/r/41428/diff/2/?file=1165618#file1165618line218>
> >
> > tabbing is off (here and below)? should be 4 spaces for continuation.
Done.
> On Dec. 16, 2015, 4:40 a.m., Maxim Khutornenko wrote:
> > src/main/python/apache/aurora/executor/common/health_checker.py, line 241
> > <https://reviews.apache.org/r/41428/diff/2/?file=1165617#file1165617line241>
> >
> > Please, create removal ticket, link it to AURORA-1559 and mention it here.
Done: AURORA-1563.
> On Dec. 16, 2015, 4:40 a.m., Maxim Khutornenko wrote:
> > src/main/python/apache/aurora/config/schema/base.py, line 39
> > <https://reviews.apache.org/r/41428/diff/2/?file=1165616#file1165616line39>
> >
> > Please, document new fields in configuration-reference.md and mark old ones as deprecated.
Done.
- Dmitriy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41428/#review110612
-----------------------------------------------------------
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
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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)
<https://reviews.apache.org/r/41428/#comment170568>
"...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)
<https://reviews.apache.org/r/41428/#comment170571>
Unfinished comments?
src/main/python/apache/aurora/client/config.py (line 97)
<https://reviews.apache.org/r/41428/#comment170572>
s/to_deprecate/deprecated
src/main/python/apache/aurora/config/schema/base.py (line 39)
<https://reviews.apache.org/r/41428/#comment170570>
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)
<https://reviews.apache.org/r/41428/#comment170575>
Please, create removal ticket, link it to AURORA-1559 and mention it here.
src/test/python/apache/aurora/client/test_config.py (line 217)
<https://reviews.apache.org/r/41428/#comment170576>
tabbing is off (here and below)? should be 4 spaces for continuation.
src/test/python/apache/aurora/client/test_config.py (line 241)
<https://reviews.apache.org/r/41428/#comment170577>
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
Posted by Dmitriy Shirchenko <ca...@gmail.com>.
-----------------------------------------------------------
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
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
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
>
>