> On May 14, 2014, 7:20 p.m., Mark Chu-Carroll wrote:
> > src/test/python/apache/aurora/client/api/test_updater.py, line 126
> > <https://reviews.apache.org/r/21440/diff/1/?file=581731#file581731line126>
> >
> >     Doesn't this mean that the test will be running with just one thread? 
> > That will serialize everything, which will mask a lot of potential error 
> > cases.

Correct. All unit tests are using a single worker thread to ensure 
verifiability. This covers the majority of the updater mechanics with real 
concurrency covered by e2e tests and runtime verification. Having multiple 
worker threads would require a huge test-rewriting effort and would result in a 
potential loss of coverage and flakiness. I would rather not do that and defer 
concurrency testing to runtime given that it's impossible to exhaustively cover 
all threading edge cases with mock unit tests.


> On May 14, 2014, 7:20 p.m., Mark Chu-Carroll wrote:
> > src/main/python/apache/aurora/client/api/updater.py, line 357
> > <https://reviews.apache.org/r/21440/diff/1/?file=581729#file581729line357>
> >
> >     What is this supposed to return?

Fixed here and in other places.


> On May 14, 2014, 7:20 p.m., Mark Chu-Carroll wrote:
> > src/main/python/apache/aurora/client/api/updater.py, line 325
> > <https://reviews.apache.org/r/21440/diff/1/?file=581729#file581729line325>
> >
> >     It took me several readthroughs to figure out what this was doing. Is 
> > there maybe a clearer way of writing this?

All other ideas I can come up with result in more code. Any ideas?


> On May 14, 2014, 7:20 p.m., Mark Chu-Carroll wrote:
> > src/main/python/apache/aurora/client/api/updater.py, line 237
> > <https://reviews.apache.org/r/21440/diff/1/?file=581729#file581729line237>
> >
> >     Why?

See reply to Brian.


> On May 14, 2014, 7:20 p.m., Mark Chu-Carroll wrote:
> > src/main/python/apache/aurora/client/api/instance_watcher.py, line 127
> > <https://reviews.apache.org/r/21440/diff/1/?file=581726#file581726line127>
> >
> >     Doc comment please. It's not clear reading this just what "terminating" 
> > or "terminate" means. It would be a huge help to just put a couple of words 
> > here.

Done.


- Maxim


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


On May 14, 2014, 6:49 p.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21440/
> -----------------------------------------------------------
> 
> (Updated May 14, 2014, 6:49 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_updater.py 
> 03a14186646daede689ac77e2ab7144aa5a7fa14 
>   src/test/python/apache/aurora/client/cli/test_update.py 
> a357c44c87065474bce47fa403ddd72625da072d 
>   src/test/python/apache/aurora/client/cli/util.py 
> 9d51a2e1fe961d4b522ac5b70f0737130326abe9 
>   src/test/python/apache/aurora/client/commands/test_update.py 
> 0b1e3498311d946631452e45ea2c7edb100dc43c 
>   src/test/python/apache/aurora/client/commands/util.py 
> fc9e2f17537e1e731381b825ac4b5646e5130654 
>   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