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/02/24 22:54:38 UTC
Review Request 18440: Adding aurora_admin sla_probe_hosts command.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18440/
-----------------------------------------------------------
Review request for Aurora, Mark Chu-Carroll and Brian Wickman.
Bugs: AURORA-209
https://issues.apache.org/jira/browse/AURORA-209
Repository: aurora
Description
-------
Implemented sla_probe_hosts command.
Diffs
-----
src/main/python/apache/aurora/client/api/sla.py fd5edd1c8dffa99fd42c0e01ecfa8ec832a4a602
src/main/python/apache/aurora/client/commands/admin.py 989c5b625b48fe67ef1297ceda8d7e35cb8ead7e
src/test/python/apache/aurora/client/api/test_sla.py 2778545b7cf42c20fb44e2fdb5b661556e84234f
src/test/python/apache/aurora/client/commands/test_admin_sla.py 780ad180366a5b8664367e499fe86470418c1344
Diff: https://reviews.apache.org/r/18440/diff/
Testing
-------
./build-support/jenkins/build.sh
Thanks,
Maxim Khutornenko
Re: Review Request 18440: Adding aurora_admin sla_probe_hosts command.
Posted by Mark Chu-Carroll <mc...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18440/#review35438
-----------------------------------------------------------
Ship it!
Ship It!
- Mark Chu-Carroll
On Feb. 25, 2014, 1:59 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18440/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2014, 1:59 p.m.)
>
>
> Review request for Aurora, Mark Chu-Carroll and Brian Wickman.
>
>
> Bugs: AURORA-209
> https://issues.apache.org/jira/browse/AURORA-209
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implemented sla_probe_hosts command.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/sla.py fd5edd1c8dffa99fd42c0e01ecfa8ec832a4a602
> src/main/python/apache/aurora/client/commands/admin.py 989c5b625b48fe67ef1297ceda8d7e35cb8ead7e
> src/test/python/apache/aurora/client/api/test_sla.py 2778545b7cf42c20fb44e2fdb5b661556e84234f
> src/test/python/apache/aurora/client/commands/test_admin_sla.py 780ad180366a5b8664367e499fe86470418c1344
>
> Diff: https://reviews.apache.org/r/18440/diff/
>
>
> Testing
> -------
>
> ./build-support/jenkins/build.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 18440: Adding aurora_admin sla_probe_hosts command.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18440/#review35440
-----------------------------------------------------------
Thanks! Pushed to master.
- Maxim Khutornenko
On Feb. 25, 2014, 6:59 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18440/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2014, 6:59 p.m.)
>
>
> Review request for Aurora, Mark Chu-Carroll and Brian Wickman.
>
>
> Bugs: AURORA-209
> https://issues.apache.org/jira/browse/AURORA-209
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implemented sla_probe_hosts command.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/sla.py fd5edd1c8dffa99fd42c0e01ecfa8ec832a4a602
> src/main/python/apache/aurora/client/commands/admin.py 989c5b625b48fe67ef1297ceda8d7e35cb8ead7e
> src/test/python/apache/aurora/client/api/test_sla.py 2778545b7cf42c20fb44e2fdb5b661556e84234f
> src/test/python/apache/aurora/client/commands/test_admin_sla.py 780ad180366a5b8664367e499fe86470418c1344
>
> Diff: https://reviews.apache.org/r/18440/diff/
>
>
> Testing
> -------
>
> ./build-support/jenkins/build.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 18440: Adding aurora_admin sla_probe_hosts command.
Posted by Brian Wickman <wi...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18440/#review35437
-----------------------------------------------------------
Ship it!
Ship It!
- Brian Wickman
On Feb. 25, 2014, 6:59 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18440/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2014, 6:59 p.m.)
>
>
> Review request for Aurora, Mark Chu-Carroll and Brian Wickman.
>
>
> Bugs: AURORA-209
> https://issues.apache.org/jira/browse/AURORA-209
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implemented sla_probe_hosts command.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/sla.py fd5edd1c8dffa99fd42c0e01ecfa8ec832a4a602
> src/main/python/apache/aurora/client/commands/admin.py 989c5b625b48fe67ef1297ceda8d7e35cb8ead7e
> src/test/python/apache/aurora/client/api/test_sla.py 2778545b7cf42c20fb44e2fdb5b661556e84234f
> src/test/python/apache/aurora/client/commands/test_admin_sla.py 780ad180366a5b8664367e499fe86470418c1344
>
> Diff: https://reviews.apache.org/r/18440/diff/
>
>
> Testing
> -------
>
> ./build-support/jenkins/build.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 18440: Adding aurora_admin sla_probe_hosts command.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18440/
-----------------------------------------------------------
(Updated Feb. 25, 2014, 6:59 p.m.)
Review request for Aurora, Mark Chu-Carroll and Brian Wickman.
Changes
-------
CR comments.
Bugs: AURORA-209
https://issues.apache.org/jira/browse/AURORA-209
Repository: aurora
Description
-------
Implemented sla_probe_hosts command.
Diffs (updated)
-----
src/main/python/apache/aurora/client/api/sla.py fd5edd1c8dffa99fd42c0e01ecfa8ec832a4a602
src/main/python/apache/aurora/client/commands/admin.py 989c5b625b48fe67ef1297ceda8d7e35cb8ead7e
src/test/python/apache/aurora/client/api/test_sla.py 2778545b7cf42c20fb44e2fdb5b661556e84234f
src/test/python/apache/aurora/client/commands/test_admin_sla.py 780ad180366a5b8664367e499fe86470418c1344
Diff: https://reviews.apache.org/r/18440/diff/
Testing
-------
./build-support/jenkins/build.sh
Thanks,
Maxim Khutornenko
Re: Review Request 18440: Adding aurora_admin sla_probe_hosts command.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18440/#review35426
-----------------------------------------------------------
src/main/python/apache/aurora/client/api/sla.py
<https://reviews.apache.org/r/18440/#comment65914>
Echo of dealing with val types in a strongly typed language :) Changed.
src/main/python/apache/aurora/client/api/sla.py
<https://reviews.apache.org/r/18440/#comment65917>
Good point. Fixed.
src/main/python/apache/aurora/client/api/sla.py
<https://reviews.apache.org/r/18440/#comment65922>
Sure, fixed.
src/main/python/apache/aurora/client/api/sla.py
<https://reviews.apache.org/r/18440/#comment65925>
Line wrapping it is. Done.
src/main/python/apache/aurora/client/commands/admin.py
<https://reviews.apache.org/r/18440/#comment65927>
Done.
src/main/python/apache/aurora/client/commands/admin.py
<https://reviews.apache.org/r/18440/#comment65928>
Was not sure about sorting applied to items(). Done.
src/main/python/apache/aurora/client/commands/admin.py
<https://reviews.apache.org/r/18440/#comment65937>
The "inf" reads "infeasible" and "infinite" - both would be right here. Also, "inf" is easier to parse from a shell script.
That said, I don't feel strong about it as "n/a" is definitely self-explanatory. Changed.
src/main/python/apache/aurora/client/commands/admin.py
<https://reviews.apache.org/r/18440/#comment65938>
Done.
- Maxim Khutornenko
On Feb. 25, 2014, 1 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18440/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2014, 1 a.m.)
>
>
> Review request for Aurora, Mark Chu-Carroll and Brian Wickman.
>
>
> Bugs: AURORA-209
> https://issues.apache.org/jira/browse/AURORA-209
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implemented sla_probe_hosts command.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/sla.py fd5edd1c8dffa99fd42c0e01ecfa8ec832a4a602
> src/main/python/apache/aurora/client/commands/admin.py 989c5b625b48fe67ef1297ceda8d7e35cb8ead7e
> src/test/python/apache/aurora/client/api/test_sla.py 2778545b7cf42c20fb44e2fdb5b661556e84234f
> src/test/python/apache/aurora/client/commands/test_admin_sla.py 780ad180366a5b8664367e499fe86470418c1344
>
> Diff: https://reviews.apache.org/r/18440/diff/
>
>
> Testing
> -------
>
> ./build-support/jenkins/build.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 18440: Adding aurora_admin sla_probe_hosts command.
Posted by Brian Wickman <wi...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18440/#review35420
-----------------------------------------------------------
logic looks sound. just minor nits.
src/main/python/apache/aurora/client/api/sla.py
<https://reviews.apache.org/r/18440/#comment65910>
rather than use -1 as a special value, it's idiomatic to use 'None' as a sentinel for meaning 'n/a' (as long as it doesn't need to be sortable, which it doesn't afaict)
src/main/python/apache/aurora/client/api/sla.py
<https://reviews.apache.org/r/18440/#comment65903>
Make sure to keep the first line of the docstring a one-liner. e.g
"""Compute predicted job SLAs following the removal of specific hosts.
For every given host creates a list of ..."""
src/main/python/apache/aurora/client/api/sla.py
<https://reviews.apache.org/r/18440/#comment65904>
why iterate by self._hosts? this guarantees runtime O(number of hosts in cluster) rather than O(number of hosts being probed)
instead do
for host in hosts:
job_keys = self._hosts.get(host, [])
...
src/main/python/apache/aurora/client/api/sla.py
<https://reviews.apache.org/r/18440/#comment65905>
it took me a while to figure out what f_ meant. prefer to just call it filtered_percentage, even if it means doing a line wrap like
filtered_percentage, task_count, filtered_vector = (
self._simulate...)
src/main/python/apache/aurora/client/commands/admin.py
<https://reviews.apache.org/r/18440/#comment65906>
+1 nl
src/main/python/apache/aurora/client/commands/admin.py
<https://reviews.apache.org/r/18440/#comment65909>
this should work fine:
for host, job_details in sorted(probed_hosts.items()):
then you can do for d in sorted(job_details)
src/main/python/apache/aurora/client/commands/admin.py
<https://reviews.apache.org/r/18440/#comment65911>
'n/a' if d.safe_in_secs is None else d.safe_in_secs
src/main/python/apache/aurora/client/commands/admin.py
<https://reviews.apache.org/r/18440/#comment65907>
kill extra newlines
- Brian Wickman
On Feb. 25, 2014, 1 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18440/
> -----------------------------------------------------------
>
> (Updated Feb. 25, 2014, 1 a.m.)
>
>
> Review request for Aurora, Mark Chu-Carroll and Brian Wickman.
>
>
> Bugs: AURORA-209
> https://issues.apache.org/jira/browse/AURORA-209
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implemented sla_probe_hosts command.
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/api/sla.py fd5edd1c8dffa99fd42c0e01ecfa8ec832a4a602
> src/main/python/apache/aurora/client/commands/admin.py 989c5b625b48fe67ef1297ceda8d7e35cb8ead7e
> src/test/python/apache/aurora/client/api/test_sla.py 2778545b7cf42c20fb44e2fdb5b661556e84234f
> src/test/python/apache/aurora/client/commands/test_admin_sla.py 780ad180366a5b8664367e499fe86470418c1344
>
> Diff: https://reviews.apache.org/r/18440/diff/
>
>
> Testing
> -------
>
> ./build-support/jenkins/build.sh
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 18440: Adding aurora_admin sla_probe_hosts command.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18440/
-----------------------------------------------------------
(Updated Feb. 25, 2014, 1 a.m.)
Review request for Aurora, Mark Chu-Carroll and Brian Wickman.
Changes
-------
When unit tests don't tell the full story until you run the thing...
Bugs: AURORA-209
https://issues.apache.org/jira/browse/AURORA-209
Repository: aurora
Description
-------
Implemented sla_probe_hosts command.
Diffs (updated)
-----
src/main/python/apache/aurora/client/api/sla.py fd5edd1c8dffa99fd42c0e01ecfa8ec832a4a602
src/main/python/apache/aurora/client/commands/admin.py 989c5b625b48fe67ef1297ceda8d7e35cb8ead7e
src/test/python/apache/aurora/client/api/test_sla.py 2778545b7cf42c20fb44e2fdb5b661556e84234f
src/test/python/apache/aurora/client/commands/test_admin_sla.py 780ad180366a5b8664367e499fe86470418c1344
Diff: https://reviews.apache.org/r/18440/diff/
Testing
-------
./build-support/jenkins/build.sh
Thanks,
Maxim Khutornenko