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



src/main/python/apache/aurora/client/api/updater.py
<https://reviews.apache.org/r/21440/#comment77715>

    This could be clearer with a bit of rephrasing in the comment:
    
    "A thread that than helps with unhandled exceptions by re-raising errors 
with the parent thread upon completion."
    



src/main/python/apache/aurora/client/api/updater.py
<https://reviews.apache.org/r/21440/#comment77717>

    I spent a bunch of time working through how this thing is doing 
threading/multiplexing. It's not actually complicated, but it's hard to follow 
if you're not familiar with the code. Documentation would be a huge help:
    
    "This method is the main body of the multiplexer thread. It repeatedly 
polls the
    worker queue for new calls, and then dispatches them in batches to the 
scheduler. Callers are notified when their requests complete."



src/main/python/apache/aurora/client/api/updater.py
<https://reviews.apache.org/r/21440/#comment77718>

    """Takes a set of RPC requests grouped by command type, dispatches them to 
the scheduler, and then waits for the batched calls to complete. When a call is 
completed, its callers will be notified via the completion queue"""



src/main/python/apache/aurora/client/api/updater.py
<https://reviews.apache.org/r/21440/#comment77716>

    I have no idea what this sentence means.
    



src/main/python/apache/aurora/client/api/updater.py
<https://reviews.apache.org/r/21440/#comment77719>

    It took me three attempts to parse this; I kept reading it as "in parallel 
with (the number of threads) limited by (batch size)"
    
    
    "Performs an update command using a collection of parallel threads. The 
number of parallel threads used is determined by the batch size parameter."



src/main/python/apache/aurora/client/api/updater.py
<https://reviews.apache.org/r/21440/#comment77720>

    Missing whitespace?



src/main/python/apache/aurora/client/api/updater.py
<https://reviews.apache.org/r/21440/#comment77721>

    A doc comment explaining the parameters is really needed here.
    



src/main/python/apache/aurora/client/api/updater.py
<https://reviews.apache.org/r/21440/#comment77722>

    Not sure I understand the point of this FailureThreshold object. It's just 
a partial view on an UpdateConfig, isn't it? Copying the fields just adds more 
indirection to the code, which is obscuring things. Better to pass the 
UpdateConfig; then it's immediately clear where these parameters are coming 
from.
    



src/main/python/apache/aurora/client/api/updater.py
<https://reviews.apache.org/r/21440/#comment77723>

    "acquiring a lock"



src/main/python/apache/aurora/client/api/updater.py
<https://reviews.apache.org/r/21440/#comment77724>

    "using a" better than "with the"



src/main/python/apache/aurora/client/api/updater.py
<https://reviews.apache.org/r/21440/#comment77725>

    I'd really prefer to see the actual logic of a single-instance update be a 
distinct method from the threading logic of pulling from queues.


- Mark Chu-Carroll


On May 15, 2014, 6:52 p.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21440/
> -----------------------------------------------------------
> 
> (Updated May 15, 2014, 6:52 p.m.)
> 
> 
> Review request for Aurora, Mark Chu-Carroll and Brian Wickman.
> 
> 
> Bugs: AURORA-350
>     https://issues.apache.org/jira/browse/AURORA-350
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> The updater now spawns upto batch_size threads to process one instance per 
> thread. 
> 
> All mutating calls are multiplexed by the SchedulerMux to do batch 
> kill/add/restart calls. This is the first step towards a fully multiplexed 
> SchedulerProxy and is intended to mitigate LDAP/scheduler load.
> 
> 
> Diffs
> -----
> 
>   src/main/python/apache/aurora/client/api/instance_watcher.py 
> 8992b93d25ce71b5784bad24429fd32356f64763 
>   src/main/python/apache/aurora/client/api/job_monitor.py 
> a27f6fbbf38e981f259aea35c9bf979d2d9b1b87 
>   src/main/python/apache/aurora/client/api/scheduler_client.py 
> ff5d2bc52af75ff2d2eee2d9b72c8d9a0df12581 
>   src/main/python/apache/aurora/client/api/updater.py 
> e8692ea4fdf924fb1ea06803d5e7321340754442 
>   src/main/python/apache/aurora/client/api/updater_util.py 
> 5ea96cd2797c4957d05168fb7ba9f950fd82bbbf 
>   src/test/python/apache/aurora/client/api/test_instance_watcher.py 
> 09dda3bed6a5097f6dff8153249a7460a5b24d58 
>   src/test/python/apache/aurora/client/api/test_job_monitor.py 
> 3df042019d845421e5c8d79232ed865e342d44fc 
>   src/test/python/apache/aurora/client/api/test_updater.py 
> 03a14186646daede689ac77e2ab7144aa5a7fa14 
>   src/test/python/apache/aurora/client/cli/test_create.py 
> ba1d1fb7bfa8cd8a48942c236039fbd2ca171a1b 
>   src/test/python/apache/aurora/client/cli/test_kill.py 
> 3c8e72de5e04b3c7dc769579506dca750b1f3089 
>   src/test/python/apache/aurora/client/cli/test_restart.py 
> 68951fa9f2f62073dee5c2b5d12ea85b44aa11db 
>   src/test/python/apache/aurora/client/cli/test_update.py 
> a357c44c87065474bce47fa403ddd72625da072d 
>   src/test/python/apache/aurora/client/cli/util.py 
> ccb6ea51eb636c2c564a0ee1a0ba2d41707bd2ca 
>   src/test/python/apache/aurora/client/commands/test_create.py 
> 09a392f3a2375d782ffdd5ab8463a0f5827c392b 
>   src/test/python/apache/aurora/client/commands/test_kill.py 
> bc3b92bdfc7838b6a0671078ef315ccd707eb5a5 
>   src/test/python/apache/aurora/client/commands/test_restart.py 
> 18e90aa8731798daac5fb5700807ebe2eb794cf9 
>   src/test/python/apache/aurora/client/commands/test_update.py 
> 0b1e3498311d946631452e45ea2c7edb100dc43c 
>   src/test/python/apache/aurora/client/commands/util.py 
> 4e587f320c785b56f5812364d50946cb2b6f03dc 
>   src/test/python/apache/aurora/client/fake_scheduler_proxy.py 
> 6a1f78b97d67adab0cf00acc31f20c5d6197a508 
>   src/test/sh/org/apache/aurora/e2e/flask/flask_example.aurora 
> ad5aafec1f796baef766ed12b2a597f26997d5b3 
>   src/test/sh/org/apache/aurora/e2e/flask/flask_example_updated.aurora 
> 6d54463c349699946c2063385b1bd38a0f8c9e58 
>   src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 
> c8fa07a5426c6c7748412062c26996b84637efb3 
>   src/test/sh/org/apache/aurora/e2e/test_end_to_end_v2.sh 
> 3c4165c8464b42f7f5dc4f94a079ff7c3086cdbc 
> 
> Diff: https://reviews.apache.org/r/21440/diff/
> 
> 
> Testing
> -------
> 
> ./pants src/test/python:all
> src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Reply via email to