> On May 14, 2014, 10:57 p.m., Brian Wickman wrote:
> > src/main/python/apache/aurora/client/api/updater.py, line 237
> > <https://reviews.apache.org/r/21440/diff/1/?file=581729#file581729line237>
> >
> >     it surprises me that this is necessary.  will signal.signal() even work 
> > outside of the MainThread?  i'm pretty sure OSes will raise an exception if 
> > you try (e.g. Linux)

You can't signal from a non-main thread but apparently any thread can catch 
KeyboardInterrupt. This wasted a few hours of my life until I ran into this 
"little" caveat 
(https://docs.python.org/2/library/thread.html?highlight=keyboardinterrupt):

"Threads interact strangely with interrupts: the KeyboardInterrupt exception 
will be received by an arbitrary thread. (When the signal module is available, 
interrupts always go to the main thread.)"


> On May 14, 2014, 10:57 p.m., Brian Wickman wrote:
> > src/main/python/apache/aurora/client/api/updater.py, line 224
> > <https://reviews.apache.org/r/21440/diff/1/?file=581729#file581729line224>
> >
> >     snake_case please

Done.


> On May 14, 2014, 10:57 p.m., Brian Wickman wrote:
> > src/main/python/apache/aurora/client/api/updater.py, line 64
> > <https://reviews.apache.org/r/21440/diff/1/?file=581729#file581729line64>
> >
> >     since you're creating unbounded queues, this is equivalent to .put() 
> > (here and elsewhere)

Changed.


> On May 14, 2014, 10:57 p.m., Brian Wickman wrote:
> > src/main/python/apache/aurora/client/api/updater.py, line 23
> > <https://reviews.apache.org/r/21440/diff/1/?file=581729#file581729line23>
> >
> >     minor nit, this should be
> >     
> >     try:
> >       from Queue import Queue, Empty
> >     except ImportError:
> >       from queue import Queue, Empty
> >     
> >     (yay 2.x/3.x)

Done.


> On May 14, 2014, 10:57 p.m., Brian Wickman wrote:
> > src/main/python/apache/aurora/client/api/instance_watcher.py, line 97
> > <https://reviews.apache.org/r/21440/diff/1/?file=581726#file581726line97>
> >
> >     while not self._terminating:

Done.


> On May 14, 2014, 10:57 p.m., Brian Wickman wrote:
> > src/main/python/apache/aurora/client/api/job_monitor.py, lines 93-94
> > <https://reviews.apache.org/r/21440/diff/1/?file=581727#file581727line93>
> >
> >     the reason that I like self._terminating to be a threading.Event is 
> > because then you could do:
> >     
> >     self._terminating.wait(poll_interval.as_(Time.SECONDS))
> >     
> >     which means that you will break out of the loop fast if 
> > self.terminate() is called from another thread.

Thanks, I see now what you meant. Great suggestion. Done.


- Maxim


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


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