> On May 20, 2014, 8:04 p.m., Mark Chu-Carroll wrote: > > src/main/python/apache/aurora/client/api/updater.py, line 58 > > <https://reviews.apache.org/r/21440/diff/2/?file=582780#file582780line58> > > > > 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." > >
Done. > On May 20, 2014, 8:04 p.m., Mark Chu-Carroll wrote: > > src/main/python/apache/aurora/client/api/updater.py, line 114 > > <https://reviews.apache.org/r/21440/diff/2/?file=582780#file582780line114> > > > > 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." Done. > On May 20, 2014, 8:04 p.m., Mark Chu-Carroll wrote: > > src/main/python/apache/aurora/client/api/updater.py, line 125 > > <https://reviews.apache.org/r/21440/diff/2/?file=582780#file582780line125> > > > > """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""" Done. > On May 20, 2014, 8:04 p.m., Mark Chu-Carroll wrote: > > src/main/python/apache/aurora/client/api/updater.py, line 140 > > <https://reviews.apache.org/r/21440/diff/2/?file=582780#file582780line140> > > > > I have no idea what this sentence means. > > Dropped. > On May 20, 2014, 8:04 p.m., Mark Chu-Carroll wrote: > > src/main/python/apache/aurora/client/api/updater.py, line 162 > > <https://reviews.apache.org/r/21440/diff/2/?file=582780#file582780line162> > > > > Missing whitespace? where? > On May 20, 2014, 8:04 p.m., Mark Chu-Carroll wrote: > > src/main/python/apache/aurora/client/api/updater.py, line 156 > > <https://reviews.apache.org/r/21440/diff/2/?file=582780#file582780line156> > > > > 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." Done. > On May 20, 2014, 8:04 p.m., Mark Chu-Carroll wrote: > > src/main/python/apache/aurora/client/api/updater.py, line 169 > > <https://reviews.apache.org/r/21440/diff/2/?file=582780#file582780line169> > > > > A doc comment explaining the parameters is really needed here. > > Documenting initializer parameters does not seem to be a common practice under api. Besides, almost all of the parameters are for unit testing purposes and normally are not expected to be specified. > On May 20, 2014, 8:04 p.m., Mark Chu-Carroll wrote: > > src/main/python/apache/aurora/client/api/updater.py, line 192 > > <https://reviews.apache.org/r/21440/diff/2/?file=582780#file582780line192> > > > > 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. > > Not sure what you proposal is. The FailureThreashold is a mutable custom class defined alongside the UpdateConfig to track failure counts. Don't see any benefit in coupling the two. > On May 20, 2014, 8:04 p.m., Mark Chu-Carroll wrote: > > src/main/python/apache/aurora/client/api/updater.py, line 205 > > <https://reviews.apache.org/r/21440/diff/2/?file=582780#file582780line205> > > > > "acquiring a lock" I actually prefer the current wording as it highlights the exclusive nature of the lock. > On May 20, 2014, 8:04 p.m., Mark Chu-Carroll wrote: > > src/main/python/apache/aurora/client/api/updater.py, line 234 > > <https://reviews.apache.org/r/21440/diff/2/?file=582780#file582780line234> > > > > "using a" better than "with the" Done. > On May 20, 2014, 8:04 p.m., Mark Chu-Carroll wrote: > > src/main/python/apache/aurora/client/api/updater.py, line 334 > > <https://reviews.apache.org/r/21440/diff/2/?file=582780#file582780line334> > > > > 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. Why? It just doubles the number of methods and dissolves the algorithm idea. This is the updater thread target method and I'd prefer to package all relevant things together. - Maxim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21440/#review43523 ----------------------------------------------------------- On May 15, 2014, 10: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, 10: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 > >
