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