aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Mark Chu-Carroll" <mchucarr...@twopensource.com>
Subject Re: Review Request 21440: Implementing parallel updater
Date Tue, 20 May 2014 20:04:52 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message