You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Maxim Khutornenko <ma...@apache.org> on 2014/05/10 03:15:10 UTC
Review Request 21297: Adding UpdateConfig value checks.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21297/
-----------------------------------------------------------
Review request for Aurora and Brian Wickman.
Bugs: AURORA-404
https://issues.apache.org/jira/browse/AURORA-404
Repository: aurora
Description
-------
Adding checks for watch_secs, initial_interval_secs and restart_threshold.
Diffs
-----
src/main/python/apache/aurora/client/config.py 350d84c97fac9f1dd834bf509a160faaacb26386
src/main/python/apache/aurora/config/__init__.py 74e6b21ef905221cb3beb974a540325937480f2e
src/main/python/apache/aurora/config/schema/base.py 61a6680f8b1a463055ccd6d318cc540aca166684
src/test/python/apache/aurora/client/cli/util.py e17f256fc5d17f251e8f2cb4d94233d2d6c09897
src/test/python/apache/aurora/client/commands/util.py c28096fe89c9af6d5518bc0f8f871e886ca12e7f
src/test/python/apache/aurora/client/test_config.py 8ef08685c317c3f9dae799dfb6bdced7077a8778
Diff: https://reviews.apache.org/r/21297/diff/
Testing
-------
./pants src/test/python:all
Thanks,
Maxim Khutornenko
Re: Review Request 21297: Adding UpdateConfig value checks.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On May 13, 2014, 9:19 p.m., Brian Wickman wrote:
> > src/main/python/apache/aurora/client/config.py, line 130
> > <https://reviews.apache.org/r/21297/diff/1/?file=578010#file578010line130>
> >
> > curious if we should be adding a delta here? if we just do watch_secs >= initial_interval_secs, then we'll race with that first health check delivery from the executor -> scheduler -> client.
> >
> > 5s? not sure. what do you think?
I was split on that too. Feels like the primary goal here should be raising awareness rather than giving any guarantees. That said, 5s might be a nice to have safety default to provide. Done.
> On May 13, 2014, 9:19 p.m., Brian Wickman wrote:
> > src/test/python/apache/aurora/client/test_config.py, line 178
> > <https://reviews.apache.org/r/21297/diff/1/?file=578015#file578015line178>
> >
> > 2 newlines between top level tests
Done.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21297/#review42887
-----------------------------------------------------------
On May 10, 2014, 1:15 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21297/
> -----------------------------------------------------------
>
> (Updated May 10, 2014, 1:15 a.m.)
>
>
> Review request for Aurora and Brian Wickman.
>
>
> Bugs: AURORA-404
> https://issues.apache.org/jira/browse/AURORA-404
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Adding checks for watch_secs, initial_interval_secs and restart_threshold.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/config.py 350d84c97fac9f1dd834bf509a160faaacb26386
> src/main/python/apache/aurora/config/__init__.py 74e6b21ef905221cb3beb974a540325937480f2e
> src/main/python/apache/aurora/config/schema/base.py 61a6680f8b1a463055ccd6d318cc540aca166684
> src/test/python/apache/aurora/client/cli/util.py e17f256fc5d17f251e8f2cb4d94233d2d6c09897
> src/test/python/apache/aurora/client/commands/util.py c28096fe89c9af6d5518bc0f8f871e886ca12e7f
> src/test/python/apache/aurora/client/test_config.py 8ef08685c317c3f9dae799dfb6bdced7077a8778
>
> Diff: https://reviews.apache.org/r/21297/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 21297: Adding UpdateConfig value checks.
Posted by Brian Wickman <wi...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21297/#review42887
-----------------------------------------------------------
src/main/python/apache/aurora/client/config.py
<https://reviews.apache.org/r/21297/#comment76804>
curious if we should be adding a delta here? if we just do watch_secs >= initial_interval_secs, then we'll race with that first health check delivery from the executor -> scheduler -> client.
5s? not sure. what do you think?
src/test/python/apache/aurora/client/test_config.py
<https://reviews.apache.org/r/21297/#comment76805>
2 newlines between top level tests
- Brian Wickman
On May 10, 2014, 1:15 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21297/
> -----------------------------------------------------------
>
> (Updated May 10, 2014, 1:15 a.m.)
>
>
> Review request for Aurora and Brian Wickman.
>
>
> Bugs: AURORA-404
> https://issues.apache.org/jira/browse/AURORA-404
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Adding checks for watch_secs, initial_interval_secs and restart_threshold.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/config.py 350d84c97fac9f1dd834bf509a160faaacb26386
> src/main/python/apache/aurora/config/__init__.py 74e6b21ef905221cb3beb974a540325937480f2e
> src/main/python/apache/aurora/config/schema/base.py 61a6680f8b1a463055ccd6d318cc540aca166684
> src/test/python/apache/aurora/client/cli/util.py e17f256fc5d17f251e8f2cb4d94233d2d6c09897
> src/test/python/apache/aurora/client/commands/util.py c28096fe89c9af6d5518bc0f8f871e886ca12e7f
> src/test/python/apache/aurora/client/test_config.py 8ef08685c317c3f9dae799dfb6bdced7077a8778
>
> Diff: https://reviews.apache.org/r/21297/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 21297: Adding UpdateConfig value checks.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21297/#review42769
-----------------------------------------------------------
Ping, Brian.
- Maxim Khutornenko
On May 10, 2014, 1:15 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21297/
> -----------------------------------------------------------
>
> (Updated May 10, 2014, 1:15 a.m.)
>
>
> Review request for Aurora and Brian Wickman.
>
>
> Bugs: AURORA-404
> https://issues.apache.org/jira/browse/AURORA-404
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Adding checks for watch_secs, initial_interval_secs and restart_threshold.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/config.py 350d84c97fac9f1dd834bf509a160faaacb26386
> src/main/python/apache/aurora/config/__init__.py 74e6b21ef905221cb3beb974a540325937480f2e
> src/main/python/apache/aurora/config/schema/base.py 61a6680f8b1a463055ccd6d318cc540aca166684
> src/test/python/apache/aurora/client/cli/util.py e17f256fc5d17f251e8f2cb4d94233d2d6c09897
> src/test/python/apache/aurora/client/commands/util.py c28096fe89c9af6d5518bc0f8f871e886ca12e7f
> src/test/python/apache/aurora/client/test_config.py 8ef08685c317c3f9dae799dfb6bdced7077a8778
>
> Diff: https://reviews.apache.org/r/21297/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 21297: Adding UpdateConfig value checks.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On May 14, 2014, 8:32 p.m., Brian Wickman wrote:
> > src/main/python/apache/aurora/client/config.py, lines 131-133
> > <https://reviews.apache.org/r/21297/diff/2-3/?file=580996#file580996line131>
> >
> > Rather than spell out a formula which may be a little intimidating, could you say something more consumable like:
> >
> > "You have specified an insufficiently long watch period (%d seconds) in your update configuration. Your update will always succeed.
> >
> > In order for health notifications to be delivered, UpdateConfig.watch_secs must be set to at least %d seconds. This corresponds to an initial health check interval (%d seconds) plus %d consecutive failures at a check interval of %d seconds."
> >
> > With blanks filled in appropriately.
> >
> > Agree y/n?
Thanks for shaping it up so nicely. Unit test output:
..CRITICAL:root:
You have specified an insufficiently short watch period (10 seconds) in your update configuration.
Your update will always succeed. In order for health notifications to be delivered,
UpdateConfig.watch_secs must be set to at least 15 seconds. This corresponds to an initial
health check interval (15 seconds) plus 0 consecutive failures at a check interval of 10 seconds.
.CRITICAL:root:
You have specified an insufficiently short watch period (20 seconds) in your update configuration.
Your update will always succeed. In order for health notifications to be delivered,
UpdateConfig.watch_secs must be set to at least 25 seconds. This corresponds to an initial
health check interval (15 seconds) plus 1 consecutive failures at a check interval of 10 seconds.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21297/#review43014
-----------------------------------------------------------
On May 14, 2014, 8:01 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21297/
> -----------------------------------------------------------
>
> (Updated May 14, 2014, 8:01 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Brian Wickman.
>
>
> Bugs: AURORA-404
> https://issues.apache.org/jira/browse/AURORA-404
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Adding checks for watch_secs, initial_interval_secs and restart_threshold.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/config.py 350d84c97fac9f1dd834bf509a160faaacb26386
> src/main/python/apache/aurora/config/__init__.py 74e6b21ef905221cb3beb974a540325937480f2e
> src/main/python/apache/aurora/config/schema/base.py 61a6680f8b1a463055ccd6d318cc540aca166684
> src/test/python/apache/aurora/client/cli/util.py e17f256fc5d17f251e8f2cb4d94233d2d6c09897
> src/test/python/apache/aurora/client/commands/util.py c28096fe89c9af6d5518bc0f8f871e886ca12e7f
> src/test/python/apache/aurora/client/test_config.py 8ef08685c317c3f9dae799dfb6bdced7077a8778
>
> Diff: https://reviews.apache.org/r/21297/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 21297: Adding UpdateConfig value checks.
Posted by Brian Wickman <wi...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21297/#review43014
-----------------------------------------------------------
src/main/python/apache/aurora/client/config.py
<https://reviews.apache.org/r/21297/#comment77000>
Rather than spell out a formula which may be a little intimidating, could you say something more consumable like:
"You have specified an insufficiently long watch period (%d seconds) in your update configuration. Your update will always succeed.
In order for health notifications to be delivered, UpdateConfig.watch_secs must be set to at least %d seconds. This corresponds to an initial health check interval (%d seconds) plus %d consecutive failures at a check interval of %d seconds."
With blanks filled in appropriately.
Agree y/n?
- Brian Wickman
On May 14, 2014, 8:01 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21297/
> -----------------------------------------------------------
>
> (Updated May 14, 2014, 8:01 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Brian Wickman.
>
>
> Bugs: AURORA-404
> https://issues.apache.org/jira/browse/AURORA-404
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Adding checks for watch_secs, initial_interval_secs and restart_threshold.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/config.py 350d84c97fac9f1dd834bf509a160faaacb26386
> src/main/python/apache/aurora/config/__init__.py 74e6b21ef905221cb3beb974a540325937480f2e
> src/main/python/apache/aurora/config/schema/base.py 61a6680f8b1a463055ccd6d318cc540aca166684
> src/test/python/apache/aurora/client/cli/util.py e17f256fc5d17f251e8f2cb4d94233d2d6c09897
> src/test/python/apache/aurora/client/commands/util.py c28096fe89c9af6d5518bc0f8f871e886ca12e7f
> src/test/python/apache/aurora/client/test_config.py 8ef08685c317c3f9dae799dfb6bdced7077a8778
>
> Diff: https://reviews.apache.org/r/21297/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 21297: Adding UpdateConfig value checks.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On May 15, 2014, 12:27 a.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/client/config.py, line 132
> > <https://reviews.apache.org/r/21297/diff/5/?file=582112#file582112line132>
> >
> > I find "health notifications to be delivered" to be vague. I prefer the wording "In order for the updater to detect health check failures"
Reworded.
> On May 15, 2014, 12:27 a.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/client/config.py, line 138
> > <https://reviews.apache.org/r/21297/diff/5/?file=582112#file582112line138>
> >
> > It would be great if you could provide documentation with guidance for setting these values. The error message can link to the documentation.
> >
> > Writing this documentation now while the caveats are fresh in your head would be a great benefit to all of our users.
I'd rather fix AURORA-405 first to avoid inconsistencies in the error messages. Created AURORA-426 to track this.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21297/#review43060
-----------------------------------------------------------
On May 14, 2014, 11:46 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21297/
> -----------------------------------------------------------
>
> (Updated May 14, 2014, 11:46 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Brian Wickman.
>
>
> Bugs: AURORA-404
> https://issues.apache.org/jira/browse/AURORA-404
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Adding checks for watch_secs, initial_interval_secs and restart_threshold.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/config.py 350d84c97fac9f1dd834bf509a160faaacb26386
> src/main/python/apache/aurora/config/__init__.py 74e6b21ef905221cb3beb974a540325937480f2e
> src/main/python/apache/aurora/config/schema/base.py 61a6680f8b1a463055ccd6d318cc540aca166684
> src/test/python/apache/aurora/client/cli/util.py e17f256fc5d17f251e8f2cb4d94233d2d6c09897
> src/test/python/apache/aurora/client/commands/util.py c28096fe89c9af6d5518bc0f8f871e886ca12e7f
> src/test/python/apache/aurora/client/test_config.py 8ef08685c317c3f9dae799dfb6bdced7077a8778
>
> Diff: https://reviews.apache.org/r/21297/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 21297: Adding UpdateConfig value checks.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21297/#review43060
-----------------------------------------------------------
Ship it!
Code looks good to me. Please seriously consider adding documentation.
src/main/python/apache/aurora/client/config.py
<https://reviews.apache.org/r/21297/#comment77081>
I find "health notifications to be delivered" to be vague. I prefer the wording "In order for the updater to detect health check failures"
src/main/python/apache/aurora/client/config.py
<https://reviews.apache.org/r/21297/#comment77082>
It would be great if you could provide documentation with guidance for setting these values. The error message can link to the documentation.
Writing this documentation now while the caveats are fresh in your head would be a great benefit to all of our users.
- Kevin Sweeney
On May 14, 2014, 4:46 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21297/
> -----------------------------------------------------------
>
> (Updated May 14, 2014, 4:46 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Brian Wickman.
>
>
> Bugs: AURORA-404
> https://issues.apache.org/jira/browse/AURORA-404
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Adding checks for watch_secs, initial_interval_secs and restart_threshold.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/config.py 350d84c97fac9f1dd834bf509a160faaacb26386
> src/main/python/apache/aurora/config/__init__.py 74e6b21ef905221cb3beb974a540325937480f2e
> src/main/python/apache/aurora/config/schema/base.py 61a6680f8b1a463055ccd6d318cc540aca166684
> src/test/python/apache/aurora/client/cli/util.py e17f256fc5d17f251e8f2cb4d94233d2d6c09897
> src/test/python/apache/aurora/client/commands/util.py c28096fe89c9af6d5518bc0f8f871e886ca12e7f
> src/test/python/apache/aurora/client/test_config.py 8ef08685c317c3f9dae799dfb6bdced7077a8778
>
> Diff: https://reviews.apache.org/r/21297/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 21297: Adding UpdateConfig value checks.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On May 15, 2014, 12:11 a.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/client/config.py, line 161
> > <https://reviews.apache.org/r/21297/diff/5/?file=582112#file582112line161>
> >
> > Also worth noting is that this warning is meaningless if the user doesn't have health checking enabled.
I think we should actually warn them regardless. Not using "health" port should rather be an exception and this warning would rather be a reminder what they are missing out.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21297/#review43056
-----------------------------------------------------------
On May 14, 2014, 11:46 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21297/
> -----------------------------------------------------------
>
> (Updated May 14, 2014, 11:46 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Brian Wickman.
>
>
> Bugs: AURORA-404
> https://issues.apache.org/jira/browse/AURORA-404
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Adding checks for watch_secs, initial_interval_secs and restart_threshold.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/config.py 350d84c97fac9f1dd834bf509a160faaacb26386
> src/main/python/apache/aurora/config/__init__.py 74e6b21ef905221cb3beb974a540325937480f2e
> src/main/python/apache/aurora/config/schema/base.py 61a6680f8b1a463055ccd6d318cc540aca166684
> src/test/python/apache/aurora/client/cli/util.py e17f256fc5d17f251e8f2cb4d94233d2d6c09897
> src/test/python/apache/aurora/client/commands/util.py c28096fe89c9af6d5518bc0f8f871e886ca12e7f
> src/test/python/apache/aurora/client/test_config.py 8ef08685c317c3f9dae799dfb6bdced7077a8778
>
> Diff: https://reviews.apache.org/r/21297/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 21297: Adding UpdateConfig value checks.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21297/#review43056
-----------------------------------------------------------
src/main/python/apache/aurora/client/config.py
<https://reviews.apache.org/r/21297/#comment77077>
Also worth noting is that this warning is meaningless if the user doesn't have health checking enabled.
- Kevin Sweeney
On May 14, 2014, 4:46 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21297/
> -----------------------------------------------------------
>
> (Updated May 14, 2014, 4:46 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Brian Wickman.
>
>
> Bugs: AURORA-404
> https://issues.apache.org/jira/browse/AURORA-404
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Adding checks for watch_secs, initial_interval_secs and restart_threshold.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/config.py 350d84c97fac9f1dd834bf509a160faaacb26386
> src/main/python/apache/aurora/config/__init__.py 74e6b21ef905221cb3beb974a540325937480f2e
> src/main/python/apache/aurora/config/schema/base.py 61a6680f8b1a463055ccd6d318cc540aca166684
> src/test/python/apache/aurora/client/cli/util.py e17f256fc5d17f251e8f2cb4d94233d2d6c09897
> src/test/python/apache/aurora/client/commands/util.py c28096fe89c9af6d5518bc0f8f871e886ca12e7f
> src/test/python/apache/aurora/client/test_config.py 8ef08685c317c3f9dae799dfb6bdced7077a8778
>
> Diff: https://reviews.apache.org/r/21297/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 21297: Adding UpdateConfig value checks.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21297/
-----------------------------------------------------------
(Updated May 15, 2014, 12:59 a.m.)
Review request for Aurora, Kevin Sweeney and Brian Wickman.
Changes
-------
CR comments.
Bugs: AURORA-404
https://issues.apache.org/jira/browse/AURORA-404
Repository: aurora
Description
-------
Adding checks for watch_secs, initial_interval_secs and restart_threshold.
Diffs (updated)
-----
src/main/python/apache/aurora/client/config.py 350d84c97fac9f1dd834bf509a160faaacb26386
src/main/python/apache/aurora/config/__init__.py 74e6b21ef905221cb3beb974a540325937480f2e
src/main/python/apache/aurora/config/schema/base.py 61a6680f8b1a463055ccd6d318cc540aca166684
src/test/python/apache/aurora/client/cli/util.py e17f256fc5d17f251e8f2cb4d94233d2d6c09897
src/test/python/apache/aurora/client/commands/util.py c28096fe89c9af6d5518bc0f8f871e886ca12e7f
src/test/python/apache/aurora/client/test_config.py 8ef08685c317c3f9dae799dfb6bdced7077a8778
Diff: https://reviews.apache.org/r/21297/diff/
Testing
-------
./pants src/test/python:all
Thanks,
Maxim Khutornenko
Re: Review Request 21297: Adding UpdateConfig value checks.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21297/
-----------------------------------------------------------
(Updated May 14, 2014, 11:46 p.m.)
Review request for Aurora, Kevin Sweeney and Brian Wickman.
Changes
-------
CR comments.
Bugs: AURORA-404
https://issues.apache.org/jira/browse/AURORA-404
Repository: aurora
Description
-------
Adding checks for watch_secs, initial_interval_secs and restart_threshold.
Diffs (updated)
-----
src/main/python/apache/aurora/client/config.py 350d84c97fac9f1dd834bf509a160faaacb26386
src/main/python/apache/aurora/config/__init__.py 74e6b21ef905221cb3beb974a540325937480f2e
src/main/python/apache/aurora/config/schema/base.py 61a6680f8b1a463055ccd6d318cc540aca166684
src/test/python/apache/aurora/client/cli/util.py e17f256fc5d17f251e8f2cb4d94233d2d6c09897
src/test/python/apache/aurora/client/commands/util.py c28096fe89c9af6d5518bc0f8f871e886ca12e7f
src/test/python/apache/aurora/client/test_config.py 8ef08685c317c3f9dae799dfb6bdced7077a8778
Diff: https://reviews.apache.org/r/21297/diff/
Testing
-------
./pants src/test/python:all
Thanks,
Maxim Khutornenko
Re: Review Request 21297: Adding UpdateConfig value checks.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On May 14, 2014, 10:55 p.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/client/config.py, line 161
> > <https://reviews.apache.org/r/21297/diff/4/?file=581994#file581994line161>
> >
> > How about adding a couple-second delay to account for executor bootup time. Ideally we'd be able to know when the executor started its stopwatch but in absence of that information we can fuzz it with a few second sleep.
I am afraid that would be hard to explain and is not quite deterministic. There is always a delay in status propagation that can wary from a few seconds to infinity. It's hard to say if 2 seconds is going to cover the majority of all cases. I think we should raise awareness here rather than provide any guarantees.
How about making it "greater than" instead of "at least" requirement? That would give us at least 1 second slack on the watch side and stimulate users to think harder about their values.
Here is the updated test output:
...CRITICAL:root:
You have specified an insufficiently short watch period (10 seconds) in your update configuration.
Your update will always succeed. In order for health notifications to be delivered,
UpdateConfig.watch_secs must be greater than 15 seconds to account for an initial
health check interval (15 seconds) plus 0 consecutive failures at a check interval of 10 seconds.
.CRITICAL:root:
You have specified an insufficiently short watch period (25 seconds) in your update configuration.
Your update will always succeed. In order for health notifications to be delivered,
UpdateConfig.watch_secs must be greater than 25 seconds to account for an initial
health check interval (15 seconds) plus 1 consecutive failures at a check interval of 10 seconds.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21297/#review43048
-----------------------------------------------------------
On May 14, 2014, 10:35 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21297/
> -----------------------------------------------------------
>
> (Updated May 14, 2014, 10:35 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Brian Wickman.
>
>
> Bugs: AURORA-404
> https://issues.apache.org/jira/browse/AURORA-404
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Adding checks for watch_secs, initial_interval_secs and restart_threshold.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/config.py 350d84c97fac9f1dd834bf509a160faaacb26386
> src/main/python/apache/aurora/config/__init__.py 74e6b21ef905221cb3beb974a540325937480f2e
> src/main/python/apache/aurora/config/schema/base.py 61a6680f8b1a463055ccd6d318cc540aca166684
> src/test/python/apache/aurora/client/cli/util.py e17f256fc5d17f251e8f2cb4d94233d2d6c09897
> src/test/python/apache/aurora/client/commands/util.py c28096fe89c9af6d5518bc0f8f871e886ca12e7f
> src/test/python/apache/aurora/client/test_config.py 8ef08685c317c3f9dae799dfb6bdced7077a8778
>
> Diff: https://reviews.apache.org/r/21297/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 21297: Adding UpdateConfig value checks.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21297/#review43048
-----------------------------------------------------------
src/main/python/apache/aurora/client/config.py
<https://reviews.apache.org/r/21297/#comment77053>
How about adding a couple-second delay to account for executor bootup time. Ideally we'd be able to know when the executor started its stopwatch but in absence of that information we can fuzz it with a few second sleep.
- Kevin Sweeney
On May 14, 2014, 3:35 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21297/
> -----------------------------------------------------------
>
> (Updated May 14, 2014, 3:35 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Brian Wickman.
>
>
> Bugs: AURORA-404
> https://issues.apache.org/jira/browse/AURORA-404
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Adding checks for watch_secs, initial_interval_secs and restart_threshold.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/config.py 350d84c97fac9f1dd834bf509a160faaacb26386
> src/main/python/apache/aurora/config/__init__.py 74e6b21ef905221cb3beb974a540325937480f2e
> src/main/python/apache/aurora/config/schema/base.py 61a6680f8b1a463055ccd6d318cc540aca166684
> src/test/python/apache/aurora/client/cli/util.py e17f256fc5d17f251e8f2cb4d94233d2d6c09897
> src/test/python/apache/aurora/client/commands/util.py c28096fe89c9af6d5518bc0f8f871e886ca12e7f
> src/test/python/apache/aurora/client/test_config.py 8ef08685c317c3f9dae799dfb6bdced7077a8778
>
> Diff: https://reviews.apache.org/r/21297/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 21297: Adding UpdateConfig value checks.
Posted by Brian Wickman <wi...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21297/#review43043
-----------------------------------------------------------
Ship it!
Ship It!
- Brian Wickman
On May 14, 2014, 10:35 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21297/
> -----------------------------------------------------------
>
> (Updated May 14, 2014, 10:35 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Brian Wickman.
>
>
> Bugs: AURORA-404
> https://issues.apache.org/jira/browse/AURORA-404
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Adding checks for watch_secs, initial_interval_secs and restart_threshold.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/config.py 350d84c97fac9f1dd834bf509a160faaacb26386
> src/main/python/apache/aurora/config/__init__.py 74e6b21ef905221cb3beb974a540325937480f2e
> src/main/python/apache/aurora/config/schema/base.py 61a6680f8b1a463055ccd6d318cc540aca166684
> src/test/python/apache/aurora/client/cli/util.py e17f256fc5d17f251e8f2cb4d94233d2d6c09897
> src/test/python/apache/aurora/client/commands/util.py c28096fe89c9af6d5518bc0f8f871e886ca12e7f
> src/test/python/apache/aurora/client/test_config.py 8ef08685c317c3f9dae799dfb6bdced7077a8778
>
> Diff: https://reviews.apache.org/r/21297/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 21297: Adding UpdateConfig value checks.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21297/
-----------------------------------------------------------
(Updated May 14, 2014, 10:35 p.m.)
Review request for Aurora, Kevin Sweeney and Brian Wickman.
Changes
-------
CR comments.
Bugs: AURORA-404
https://issues.apache.org/jira/browse/AURORA-404
Repository: aurora
Description
-------
Adding checks for watch_secs, initial_interval_secs and restart_threshold.
Diffs (updated)
-----
src/main/python/apache/aurora/client/config.py 350d84c97fac9f1dd834bf509a160faaacb26386
src/main/python/apache/aurora/config/__init__.py 74e6b21ef905221cb3beb974a540325937480f2e
src/main/python/apache/aurora/config/schema/base.py 61a6680f8b1a463055ccd6d318cc540aca166684
src/test/python/apache/aurora/client/cli/util.py e17f256fc5d17f251e8f2cb4d94233d2d6c09897
src/test/python/apache/aurora/client/commands/util.py c28096fe89c9af6d5518bc0f8f871e886ca12e7f
src/test/python/apache/aurora/client/test_config.py 8ef08685c317c3f9dae799dfb6bdced7077a8778
Diff: https://reviews.apache.org/r/21297/diff/
Testing
-------
./pants src/test/python:all
Thanks,
Maxim Khutornenko
Re: Review Request 21297: Adding UpdateConfig value checks.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21297/
-----------------------------------------------------------
(Updated May 14, 2014, 8:01 p.m.)
Review request for Aurora, Kevin Sweeney and Brian Wickman.
Bugs: AURORA-404
https://issues.apache.org/jira/browse/AURORA-404
Repository: aurora
Description
-------
Adding checks for watch_secs, initial_interval_secs and restart_threshold.
Diffs
-----
src/main/python/apache/aurora/client/config.py 350d84c97fac9f1dd834bf509a160faaacb26386
src/main/python/apache/aurora/config/__init__.py 74e6b21ef905221cb3beb974a540325937480f2e
src/main/python/apache/aurora/config/schema/base.py 61a6680f8b1a463055ccd6d318cc540aca166684
src/test/python/apache/aurora/client/cli/util.py e17f256fc5d17f251e8f2cb4d94233d2d6c09897
src/test/python/apache/aurora/client/commands/util.py c28096fe89c9af6d5518bc0f8f871e886ca12e7f
src/test/python/apache/aurora/client/test_config.py 8ef08685c317c3f9dae799dfb6bdced7077a8778
Diff: https://reviews.apache.org/r/21297/diff/
Testing
-------
./pants src/test/python:all
Thanks,
Maxim Khutornenko
Re: Review Request 21297: Adding UpdateConfig value checks.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21297/
-----------------------------------------------------------
(Updated May 14, 2014, 8 p.m.)
Review request for Aurora, Kevin Sweeney and Brian Wickman.
Changes
-------
Dropped 5sec buffer in favor of a watch_secs >= initial_interval_secs + (max_consecutive_failures * interval_secs). The 5secs is replaced by the extra interval_secs.
Bugs: AURORA-404
https://issues.apache.org/jira/browse/AURORA-404
Repository: aurora
Description
-------
Adding checks for watch_secs, initial_interval_secs and restart_threshold.
Diffs (updated)
-----
src/main/python/apache/aurora/client/config.py 350d84c97fac9f1dd834bf509a160faaacb26386
src/main/python/apache/aurora/config/__init__.py 74e6b21ef905221cb3beb974a540325937480f2e
src/main/python/apache/aurora/config/schema/base.py 61a6680f8b1a463055ccd6d318cc540aca166684
src/test/python/apache/aurora/client/cli/util.py e17f256fc5d17f251e8f2cb4d94233d2d6c09897
src/test/python/apache/aurora/client/commands/util.py c28096fe89c9af6d5518bc0f8f871e886ca12e7f
src/test/python/apache/aurora/client/test_config.py 8ef08685c317c3f9dae799dfb6bdced7077a8778
Diff: https://reviews.apache.org/r/21297/diff/
Testing
-------
./pants src/test/python:all
Thanks,
Maxim Khutornenko
Re: Review Request 21297: Adding UpdateConfig value checks.
Posted by Brian Wickman <wi...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21297/#review42914
-----------------------------------------------------------
Ship it!
src/main/python/apache/aurora/client/config.py
<https://reviews.apache.org/r/21297/#comment76845>
make this a named constant somewhere, e.g. STATUS_UPDATE_DELIVERY_WINDOW
just so it's more self documenting to a casual reader. i guess this will also mean you need to parameterize it in WATCH_SECS_INSUFFICIENT_ERROR
- Brian Wickman
On May 13, 2014, 10:33 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21297/
> -----------------------------------------------------------
>
> (Updated May 13, 2014, 10:33 p.m.)
>
>
> Review request for Aurora and Brian Wickman.
>
>
> Bugs: AURORA-404
> https://issues.apache.org/jira/browse/AURORA-404
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Adding checks for watch_secs, initial_interval_secs and restart_threshold.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/config.py 350d84c97fac9f1dd834bf509a160faaacb26386
> src/main/python/apache/aurora/config/__init__.py 74e6b21ef905221cb3beb974a540325937480f2e
> src/main/python/apache/aurora/config/schema/base.py 61a6680f8b1a463055ccd6d318cc540aca166684
> src/test/python/apache/aurora/client/cli/util.py e17f256fc5d17f251e8f2cb4d94233d2d6c09897
> src/test/python/apache/aurora/client/commands/util.py c28096fe89c9af6d5518bc0f8f871e886ca12e7f
> src/test/python/apache/aurora/client/test_config.py 8ef08685c317c3f9dae799dfb6bdced7077a8778
>
> Diff: https://reviews.apache.org/r/21297/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 21297: Adding UpdateConfig value checks.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21297/
-----------------------------------------------------------
(Updated May 13, 2014, 10:33 p.m.)
Review request for Aurora and Brian Wickman.
Changes
-------
CR comments.
Bugs: AURORA-404
https://issues.apache.org/jira/browse/AURORA-404
Repository: aurora
Description
-------
Adding checks for watch_secs, initial_interval_secs and restart_threshold.
Diffs (updated)
-----
src/main/python/apache/aurora/client/config.py 350d84c97fac9f1dd834bf509a160faaacb26386
src/main/python/apache/aurora/config/__init__.py 74e6b21ef905221cb3beb974a540325937480f2e
src/main/python/apache/aurora/config/schema/base.py 61a6680f8b1a463055ccd6d318cc540aca166684
src/test/python/apache/aurora/client/cli/util.py e17f256fc5d17f251e8f2cb4d94233d2d6c09897
src/test/python/apache/aurora/client/commands/util.py c28096fe89c9af6d5518bc0f8f871e886ca12e7f
src/test/python/apache/aurora/client/test_config.py 8ef08685c317c3f9dae799dfb6bdced7077a8778
Diff: https://reviews.apache.org/r/21297/diff/
Testing
-------
./pants src/test/python:all
Thanks,
Maxim Khutornenko