-----------------------------------------------------------
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
> 
>

Reply via email to