Re: Review Request 24126: Adding a wait_for_batch_completion option into parallel updater.

[Maxim Khutornenko]

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

(Updated July 31, 2014, 7:59 p.m.)


Review request for Aurora, Kevin Sweeney, Mark Chu-Carroll, and Bill Farner.


Changes
---

- wickman
+ markcc


Bugs: AURORA-626
https://issues.apache.org/jira/browse/AURORA-626


Repository: aurora


Description
---

Adding a wait_for_batch_completion option into parallel updater.


Diffs
-

  src/main/python/apache/aurora/client/api/updater.py 
05b4c0c76ac2a8551f3aa370ab487f9f0802c3dc 
  src/main/python/apache/aurora/client/api/updater_util.py 
c5f8f23912701568e1ee6b69186a533fdd29a5d7 
  src/main/python/apache/aurora/config/schema/base.py 
3b90ccb33e6ffef9e14befd42d95b8b8a94e949b 
  src/test/python/apache/aurora/client/api/test_updater.py 
7020712c9f0b33ec29646482517768ccb13e881f 

Diff: https://reviews.apache.org/r/24126/diff/


Testing
---

./pants src/test/python:all


Thanks,

Maxim Khutornenko

Re: Review Request 24126: Adding a wait_for_batch_completion option into parallel updater.

[Mark Chu-Carroll]

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

Ship it!


Ship It!

- Mark Chu-Carroll


On July 31, 2014, 3:34 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24126/
> ---
> 
> (Updated July 31, 2014, 3:34 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Bill Farner, and Brian Wickman.
> 
> 
> Bugs: AURORA-626
> https://issues.apache.org/jira/browse/AURORA-626
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding a wait_for_batch_completion option into parallel updater.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/updater.py 
> 05b4c0c76ac2a8551f3aa370ab487f9f0802c3dc 
>   src/main/python/apache/aurora/client/api/updater_util.py 
> c5f8f23912701568e1ee6b69186a533fdd29a5d7 
>   src/main/python/apache/aurora/config/schema/base.py 
> 3b90ccb33e6ffef9e14befd42d95b8b8a94e949b 
>   src/test/python/apache/aurora/client/api/test_updater.py 
> 7020712c9f0b33ec29646482517768ccb13e881f 
> 
> Diff: https://reviews.apache.org/r/24126/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python:all
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 24126: Adding a wait_for_batch_completion option into parallel updater.

[Kevin Sweeney]

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

Ship it!


Ship It!

- Kevin Sweeney


On July 31, 2014, 12:34 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24126/
> ---
> 
> (Updated July 31, 2014, 12:34 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Bill Farner, and Brian Wickman.
> 
> 
> Bugs: AURORA-626
> https://issues.apache.org/jira/browse/AURORA-626
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding a wait_for_batch_completion option into parallel updater.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/updater.py 
> 05b4c0c76ac2a8551f3aa370ab487f9f0802c3dc 
>   src/main/python/apache/aurora/client/api/updater_util.py 
> c5f8f23912701568e1ee6b69186a533fdd29a5d7 
>   src/main/python/apache/aurora/config/schema/base.py 
> 3b90ccb33e6ffef9e14befd42d95b8b8a94e949b 
>   src/test/python/apache/aurora/client/api/test_updater.py 
> 7020712c9f0b33ec29646482517768ccb13e881f 
> 
> Diff: https://reviews.apache.org/r/24126/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python:all
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 24126: Adding a wait_for_batch_completion option into parallel updater.

[Maxim Khutornenko]

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

(Updated July 31, 2014, 7:34 p.m.)


Review request for Aurora, Kevin Sweeney, Bill Farner, and Brian Wickman.


Changes
---

CR comments.


Bugs: AURORA-626
https://issues.apache.org/jira/browse/AURORA-626


Repository: aurora


Description
---

Adding a wait_for_batch_completion option into parallel updater.


Diffs (updated)
-

  src/main/python/apache/aurora/client/api/updater.py 
05b4c0c76ac2a8551f3aa370ab487f9f0802c3dc 
  src/main/python/apache/aurora/client/api/updater_util.py 
c5f8f23912701568e1ee6b69186a533fdd29a5d7 
  src/main/python/apache/aurora/config/schema/base.py 
3b90ccb33e6ffef9e14befd42d95b8b8a94e949b 
  src/test/python/apache/aurora/client/api/test_updater.py 
7020712c9f0b33ec29646482517768ccb13e881f 

Diff: https://reviews.apache.org/r/24126/diff/


Testing
---

./pants src/test/python:all


Thanks,

Maxim Khutornenko

Re: Review Request 24126: Adding a wait_for_batch_completion option into parallel updater.

[Maxim Khutornenko]

> On July 31, 2014, 7:04 p.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/client/api/updater.py, line 19
> > 
> >
> > as far as I can tell you're not avoiding a name conflict here by 
> > renaming - mind just using Event?

Sure. 


> On July 31, 2014, 7:04 p.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/client/api/updater.py, lines 234-235
> > 
> >
> > I can't find any documentation that this actually works - in theory 
> > http://hg.python.org/cpython/file/e49efa892efb/Lib/threading.py#l522 should 
> > do it but that makes the assumption that after calling notify_all() the 
> > waiting threads will be immediately scheduled to see the new value of the 
> > event flag before you clear it (because if they don't see it it's a 
> > deadlock).
> > 
> > It's too bad you can't use Barrier from 3.2. 
> > 
> > Not sure I have a better solution but seems like a potentially tricky 
> > bug.
> 
> Kevin Sweeney wrote:
> Rereading it the code does seem correct, since the monitor-protected flag 
> is only checked to determine whether to wait and wait returns as soon as 
> there's a wakeup.

I doubt waiting threads are actually using this flag when awakened. From what I 
can tell, the self._flag is only relevant within the Event itself and serves as 
a check in wait(). I don't see a reason why awakened threads would actually 
need to re-check the flag, the notification call should be just enough for them 
to continue. Otherwise, it would be a perf bottleneck at best.


- Maxim


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


On July 31, 2014, 4:38 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24126/
> ---
> 
> (Updated July 31, 2014, 4:38 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Bill Farner, and Brian Wickman.
> 
> 
> Bugs: AURORA-626
> https://issues.apache.org/jira/browse/AURORA-626
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding a wait_for_batch_completion option into parallel updater.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/updater.py 
> 05b4c0c76ac2a8551f3aa370ab487f9f0802c3dc 
>   src/main/python/apache/aurora/client/api/updater_util.py 
> c5f8f23912701568e1ee6b69186a533fdd29a5d7 
>   src/main/python/apache/aurora/config/schema/base.py 
> 3b90ccb33e6ffef9e14befd42d95b8b8a94e949b 
>   src/test/python/apache/aurora/client/api/test_updater.py 
> 7020712c9f0b33ec29646482517768ccb13e881f 
> 
> Diff: https://reviews.apache.org/r/24126/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python:all
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 24126: Adding a wait_for_batch_completion option into parallel updater.

[Maxim Khutornenko]

> On July 31, 2014, 7 p.m., Mark Chu-Carroll wrote:
> > src/main/python/apache/aurora/client/api/updater.py, line 215
> > 
> >
> > I do not understand this method. The doc comment isn't clear, and the 
> > logic of it doesn't make obvious sense. 
> > 
> > (1) Under what conditions do you need to wait for the batch to 
> > complete? I'm not sure of what you're trying to capture here.
> > 
> > (2) This is side-effecting the queues, but the name of it suggests that 
> > it's just a predicate.
> >

This method is called from _wait_for_batch_completion_if_needed() and is gated 
by the correspondent UpdateConfig prop check there. The reason this method 
exists is to make the caller logic a bit simpler and to isolate the locked 
content. Reworked a bit to make it clear.


- Maxim


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


On July 31, 2014, 4:38 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24126/
> ---
> 
> (Updated July 31, 2014, 4:38 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Bill Farner, and Brian Wickman.
> 
> 
> Bugs: AURORA-626
> https://issues.apache.org/jira/browse/AURORA-626
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding a wait_for_batch_completion option into parallel updater.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/updater.py 
> 05b4c0c76ac2a8551f3aa370ab487f9f0802c3dc 
>   src/main/python/apache/aurora/client/api/updater_util.py 
> c5f8f23912701568e1ee6b69186a533fdd29a5d7 
>   src/main/python/apache/aurora/config/schema/base.py 
> 3b90ccb33e6ffef9e14befd42d95b8b8a94e949b 
>   src/test/python/apache/aurora/client/api/test_updater.py 
> 7020712c9f0b33ec29646482517768ccb13e881f 
> 
> Diff: https://reviews.apache.org/r/24126/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python:all
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 24126: Adding a wait_for_batch_completion option into parallel updater.

[Kevin Sweeney]

> On July 31, 2014, 12:04 p.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/client/api/updater.py, lines 234-235
> > 
> >
> > I can't find any documentation that this actually works - in theory 
> > http://hg.python.org/cpython/file/e49efa892efb/Lib/threading.py#l522 should 
> > do it but that makes the assumption that after calling notify_all() the 
> > waiting threads will be immediately scheduled to see the new value of the 
> > event flag before you clear it (because if they don't see it it's a 
> > deadlock).
> > 
> > It's too bad you can't use Barrier from 3.2. 
> > 
> > Not sure I have a better solution but seems like a potentially tricky 
> > bug.

Rereading it the code does seem correct, since the monitor-protected flag is 
only checked to determine whether to wait and wait returns as soon as there's a 
wakeup.


- Kevin


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


On July 31, 2014, 9:38 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24126/
> ---
> 
> (Updated July 31, 2014, 9:38 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Bill Farner, and Brian Wickman.
> 
> 
> Bugs: AURORA-626
> https://issues.apache.org/jira/browse/AURORA-626
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding a wait_for_batch_completion option into parallel updater.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/updater.py 
> 05b4c0c76ac2a8551f3aa370ab487f9f0802c3dc 
>   src/main/python/apache/aurora/client/api/updater_util.py 
> c5f8f23912701568e1ee6b69186a533fdd29a5d7 
>   src/main/python/apache/aurora/config/schema/base.py 
> 3b90ccb33e6ffef9e14befd42d95b8b8a94e949b 
>   src/test/python/apache/aurora/client/api/test_updater.py 
> 7020712c9f0b33ec29646482517768ccb13e881f 
> 
> Diff: https://reviews.apache.org/r/24126/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python:all
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 24126: Adding a wait_for_batch_completion option into parallel updater.

[Kevin Sweeney]

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



src/main/python/apache/aurora/client/api/updater.py


as far as I can tell you're not avoiding a name conflict here by renaming - 
mind just using Event?



src/main/python/apache/aurora/client/api/updater.py


I can't find any documentation that this actually works - in theory 
http://hg.python.org/cpython/file/e49efa892efb/Lib/threading.py#l522 should do 
it but that makes the assumption that after calling notify_all() the waiting 
threads will be immediately scheduled to see the new value of the event flag 
before you clear it (because if they don't see it it's a deadlock).

It's too bad you can't use Barrier from 3.2. 

Not sure I have a better solution but seems like a potentially tricky bug.


- Kevin Sweeney


On July 31, 2014, 9:38 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24126/
> ---
> 
> (Updated July 31, 2014, 9:38 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Bill Farner, and Brian Wickman.
> 
> 
> Bugs: AURORA-626
> https://issues.apache.org/jira/browse/AURORA-626
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding a wait_for_batch_completion option into parallel updater.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/updater.py 
> 05b4c0c76ac2a8551f3aa370ab487f9f0802c3dc 
>   src/main/python/apache/aurora/client/api/updater_util.py 
> c5f8f23912701568e1ee6b69186a533fdd29a5d7 
>   src/main/python/apache/aurora/config/schema/base.py 
> 3b90ccb33e6ffef9e14befd42d95b8b8a94e949b 
>   src/test/python/apache/aurora/client/api/test_updater.py 
> 7020712c9f0b33ec29646482517768ccb13e881f 
> 
> Diff: https://reviews.apache.org/r/24126/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python:all
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 24126: Adding a wait_for_batch_completion option into parallel updater.

[Mark Chu-Carroll]

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



src/main/python/apache/aurora/client/api/updater.py


I do not understand this method. The doc comment isn't clear, and the logic 
of it doesn't make obvious sense. 

(1) Under what conditions do you need to wait for the batch to complete? 
I'm not sure of what you're trying to capture here.

(2) This is side-effecting the queues, but the name of it suggests that 
it's just a predicate.



- Mark Chu-Carroll


On July 31, 2014, 12:38 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24126/
> ---
> 
> (Updated July 31, 2014, 12:38 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Bill Farner, and Brian Wickman.
> 
> 
> Bugs: AURORA-626
> https://issues.apache.org/jira/browse/AURORA-626
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding a wait_for_batch_completion option into parallel updater.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/updater.py 
> 05b4c0c76ac2a8551f3aa370ab487f9f0802c3dc 
>   src/main/python/apache/aurora/client/api/updater_util.py 
> c5f8f23912701568e1ee6b69186a533fdd29a5d7 
>   src/main/python/apache/aurora/config/schema/base.py 
> 3b90ccb33e6ffef9e14befd42d95b8b8a94e949b 
>   src/test/python/apache/aurora/client/api/test_updater.py 
> 7020712c9f0b33ec29646482517768ccb13e881f 
> 
> Diff: https://reviews.apache.org/r/24126/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python:all
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 24126: Adding a wait_for_batch_completion option into parallel updater.

[Bill Farner]

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

Ship it!


Ship It!

- Bill Farner


On July 31, 2014, 4:38 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24126/
> ---
> 
> (Updated July 31, 2014, 4:38 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Bill Farner, and Brian Wickman.
> 
> 
> Bugs: AURORA-626
> https://issues.apache.org/jira/browse/AURORA-626
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding a wait_for_batch_completion option into parallel updater.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/updater.py 
> 05b4c0c76ac2a8551f3aa370ab487f9f0802c3dc 
>   src/main/python/apache/aurora/client/api/updater_util.py 
> c5f8f23912701568e1ee6b69186a533fdd29a5d7 
>   src/main/python/apache/aurora/config/schema/base.py 
> 3b90ccb33e6ffef9e14befd42d95b8b8a94e949b 
>   src/test/python/apache/aurora/client/api/test_updater.py 
> 7020712c9f0b33ec29646482517768ccb13e881f 
> 
> Diff: https://reviews.apache.org/r/24126/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python:all
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 24126: Adding a wait_for_batch_completion option into parallel updater.

[Maxim Khutornenko]

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

(Updated July 31, 2014, 4:38 p.m.)


Review request for Aurora, Kevin Sweeney, Bill Farner, and Brian Wickman.


Changes
---

CR comments.


Bugs: AURORA-626
https://issues.apache.org/jira/browse/AURORA-626


Repository: aurora


Description
---

Adding a wait_for_batch_completion option into parallel updater.


Diffs (updated)
-

  src/main/python/apache/aurora/client/api/updater.py 
05b4c0c76ac2a8551f3aa370ab487f9f0802c3dc 
  src/main/python/apache/aurora/client/api/updater_util.py 
c5f8f23912701568e1ee6b69186a533fdd29a5d7 
  src/main/python/apache/aurora/config/schema/base.py 
3b90ccb33e6ffef9e14befd42d95b8b8a94e949b 
  src/test/python/apache/aurora/client/api/test_updater.py 
7020712c9f0b33ec29646482517768ccb13e881f 

Diff: https://reviews.apache.org/r/24126/diff/


Testing
---

./pants src/test/python:all


Thanks,

Maxim Khutornenko

Re: Review Request 24126: Adding a wait_for_batch_completion option into parallel updater.

[Maxim Khutornenko]

> On July 31, 2014, 12:19 a.m., Bill Farner wrote:
> > src/test/python/apache/aurora/client/api/test_updater.py, line 821
> > 
> >
> > Can you add two more test cases with batch_size > 1:
> > 
> >  - instances % batch_size == 0
> >  - instances % batch_size != 0
> 
> Maxim Khutornenko wrote:
> Unfortunately, any batch_size greater than 1 would require complete 
> refactoring of the updater unit tests and switching to python mock library. 
> Mox does not handle out of order verification properly. I have originally 
> implemented both tests but had to back out at the last moment as tests ran 
> clean only about 80% of the time with other 20% failing stub verification.
> 
> Bill Farner wrote:
> Does InAnyOrder help? 
> https://code.google.com/p/pymox/wiki/MoxDocumentation#In_Any_Order
> 
> Maxim Khutornenko wrote:
> It does not. That's exactly what I used.
> 
> Maxim Khutornenko wrote:
> Actually, to be precise it does help to get to those 80%. Without 
> AnyOrder() tests fail 100% of the time. However, something within AnyOrder() 
> races with the verification and fails in 1 out of 5 runs.

Ah, found the problem. Stupid overlook on my side - missing InAnyOrder() on the 
JobMonitor expectation. 


- Maxim


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


On July 31, 2014, 4:09 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24126/
> ---
> 
> (Updated July 31, 2014, 4:09 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Bill Farner, and Brian Wickman.
> 
> 
> Bugs: AURORA-626
> https://issues.apache.org/jira/browse/AURORA-626
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding a wait_for_batch_completion option into parallel updater.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/updater.py 
> 05b4c0c76ac2a8551f3aa370ab487f9f0802c3dc 
>   src/main/python/apache/aurora/client/api/updater_util.py 
> c5f8f23912701568e1ee6b69186a533fdd29a5d7 
>   src/main/python/apache/aurora/config/schema/base.py 
> 3b90ccb33e6ffef9e14befd42d95b8b8a94e949b 
>   src/test/python/apache/aurora/client/api/test_updater.py 
> 7020712c9f0b33ec29646482517768ccb13e881f 
> 
> Diff: https://reviews.apache.org/r/24126/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python:all
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 24126: Adding a wait_for_batch_completion option into parallel updater.

[Maxim Khutornenko]

> On July 31, 2014, 12:19 a.m., Bill Farner wrote:
> > src/test/python/apache/aurora/client/api/test_updater.py, line 821
> > 
> >
> > Can you add two more test cases with batch_size > 1:
> > 
> >  - instances % batch_size == 0
> >  - instances % batch_size != 0
> 
> Maxim Khutornenko wrote:
> Unfortunately, any batch_size greater than 1 would require complete 
> refactoring of the updater unit tests and switching to python mock library. 
> Mox does not handle out of order verification properly. I have originally 
> implemented both tests but had to back out at the last moment as tests ran 
> clean only about 80% of the time with other 20% failing stub verification.
> 
> Bill Farner wrote:
> Does InAnyOrder help? 
> https://code.google.com/p/pymox/wiki/MoxDocumentation#In_Any_Order
> 
> Maxim Khutornenko wrote:
> It does not. That's exactly what I used.

Actually, to be precise it does help to get to those 80%. Without AnyOrder() 
tests fail 100% of the time. However, something within AnyOrder() races with 
the verification and fails in 1 out of 5 runs.


- Maxim


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


On July 31, 2014, 4:09 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24126/
> ---
> 
> (Updated July 31, 2014, 4:09 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Bill Farner, and Brian Wickman.
> 
> 
> Bugs: AURORA-626
> https://issues.apache.org/jira/browse/AURORA-626
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding a wait_for_batch_completion option into parallel updater.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/updater.py 
> 05b4c0c76ac2a8551f3aa370ab487f9f0802c3dc 
>   src/main/python/apache/aurora/client/api/updater_util.py 
> c5f8f23912701568e1ee6b69186a533fdd29a5d7 
>   src/main/python/apache/aurora/config/schema/base.py 
> 3b90ccb33e6ffef9e14befd42d95b8b8a94e949b 
>   src/test/python/apache/aurora/client/api/test_updater.py 
> 7020712c9f0b33ec29646482517768ccb13e881f 
> 
> Diff: https://reviews.apache.org/r/24126/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python:all
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 24126: Adding a wait_for_batch_completion option into parallel updater.

[Maxim Khutornenko]

> On July 31, 2014, 12:19 a.m., Bill Farner wrote:
> > src/test/python/apache/aurora/client/api/test_updater.py, line 821
> > 
> >
> > Can you add two more test cases with batch_size > 1:
> > 
> >  - instances % batch_size == 0
> >  - instances % batch_size != 0
> 
> Maxim Khutornenko wrote:
> Unfortunately, any batch_size greater than 1 would require complete 
> refactoring of the updater unit tests and switching to python mock library. 
> Mox does not handle out of order verification properly. I have originally 
> implemented both tests but had to back out at the last moment as tests ran 
> clean only about 80% of the time with other 20% failing stub verification.
> 
> Bill Farner wrote:
> Does InAnyOrder help? 
> https://code.google.com/p/pymox/wiki/MoxDocumentation#In_Any_Order

It does not. That's exactly what I used.


- Maxim


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


On July 31, 2014, 4:09 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24126/
> ---
> 
> (Updated July 31, 2014, 4:09 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Bill Farner, and Brian Wickman.
> 
> 
> Bugs: AURORA-626
> https://issues.apache.org/jira/browse/AURORA-626
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding a wait_for_batch_completion option into parallel updater.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/updater.py 
> 05b4c0c76ac2a8551f3aa370ab487f9f0802c3dc 
>   src/main/python/apache/aurora/client/api/updater_util.py 
> c5f8f23912701568e1ee6b69186a533fdd29a5d7 
>   src/main/python/apache/aurora/config/schema/base.py 
> 3b90ccb33e6ffef9e14befd42d95b8b8a94e949b 
>   src/test/python/apache/aurora/client/api/test_updater.py 
> 7020712c9f0b33ec29646482517768ccb13e881f 
> 
> Diff: https://reviews.apache.org/r/24126/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python:all
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 24126: Adding a wait_for_batch_completion option into parallel updater.

[Bill Farner]

> On July 31, 2014, 12:19 a.m., Bill Farner wrote:
> > src/test/python/apache/aurora/client/api/test_updater.py, line 821
> > 
> >
> > Can you add two more test cases with batch_size > 1:
> > 
> >  - instances % batch_size == 0
> >  - instances % batch_size != 0
> 
> Maxim Khutornenko wrote:
> Unfortunately, any batch_size greater than 1 would require complete 
> refactoring of the updater unit tests and switching to python mock library. 
> Mox does not handle out of order verification properly. I have originally 
> implemented both tests but had to back out at the last moment as tests ran 
> clean only about 80% of the time with other 20% failing stub verification.

Does InAnyOrder help? 
https://code.google.com/p/pymox/wiki/MoxDocumentation#In_Any_Order


- Bill


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


On July 31, 2014, 4:09 a.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24126/
> ---
> 
> (Updated July 31, 2014, 4:09 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Bill Farner, and Brian Wickman.
> 
> 
> Bugs: AURORA-626
> https://issues.apache.org/jira/browse/AURORA-626
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding a wait_for_batch_completion option into parallel updater.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/updater.py 
> 05b4c0c76ac2a8551f3aa370ab487f9f0802c3dc 
>   src/main/python/apache/aurora/client/api/updater_util.py 
> c5f8f23912701568e1ee6b69186a533fdd29a5d7 
>   src/main/python/apache/aurora/config/schema/base.py 
> 3b90ccb33e6ffef9e14befd42d95b8b8a94e949b 
>   src/test/python/apache/aurora/client/api/test_updater.py 
> 7020712c9f0b33ec29646482517768ccb13e881f 
> 
> Diff: https://reviews.apache.org/r/24126/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python:all
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 24126: Adding a wait_for_batch_completion option into parallel updater.

[Maxim Khutornenko]

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

(Updated July 31, 2014, 4:09 a.m.)


Review request for Aurora, Kevin Sweeney, Bill Farner, and Brian Wickman.


Changes
---

CR comments.


Bugs: AURORA-626
https://issues.apache.org/jira/browse/AURORA-626


Repository: aurora


Description
---

Adding a wait_for_batch_completion option into parallel updater.


Diffs (updated)
-

  src/main/python/apache/aurora/client/api/updater.py 
05b4c0c76ac2a8551f3aa370ab487f9f0802c3dc 
  src/main/python/apache/aurora/client/api/updater_util.py 
c5f8f23912701568e1ee6b69186a533fdd29a5d7 
  src/main/python/apache/aurora/config/schema/base.py 
3b90ccb33e6ffef9e14befd42d95b8b8a94e949b 
  src/test/python/apache/aurora/client/api/test_updater.py 
7020712c9f0b33ec29646482517768ccb13e881f 

Diff: https://reviews.apache.org/r/24126/diff/


Testing
---

./pants src/test/python:all


Thanks,

Maxim Khutornenko

Re: Review Request 24126: Adding a wait_for_batch_completion option into parallel updater.

[Maxim Khutornenko]

> On July 31, 2014, 12:19 a.m., Bill Farner wrote:
> > src/main/python/apache/aurora/client/api/updater.py, line 242
> > 
> >
> > Nit - this flag is only gated on "if not".  Consider inverting the 
> > meaning of the flag and don't negate.

The default (False) behavior is to not wait for completion. The current name 
feels right to me as the feature is not ON by default.


> On July 31, 2014, 12:19 a.m., Bill Farner wrote:
> > src/main/python/apache/aurora/client/api/updater.py, line 245
> > 
> >
> > Ditto - gate is always negative.  Consider inverting the meaning and 
> > return value of the function.

Renamed.


> On July 31, 2014, 12:19 a.m., Bill Farner wrote:
> > src/main/python/apache/aurora/client/api/updater.py, line 247
> > 
> >
> > s/Wait/Waiting/

done.


> On July 31, 2014, 12:19 a.m., Bill Farner wrote:
> > src/test/python/apache/aurora/client/api/test_updater.py, line 821
> > 
> >
> > Can you add two more test cases with batch_size > 1:
> > 
> >  - instances % batch_size == 0
> >  - instances % batch_size != 0

Unfortunately, any batch_size greater than 1 would require complete refactoring 
of the updater unit tests and switching to python mock library. Mox does not 
handle out of order verification properly. I have originally implemented both 
tests but had to back out at the last moment as tests ran clean only about 80% 
of the time with other 20% failing stub verification.  


- Maxim


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


On July 30, 2014, 11:59 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24126/
> ---
> 
> (Updated July 30, 2014, 11:59 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Bill Farner, and Brian Wickman.
> 
> 
> Bugs: AURORA-626
> https://issues.apache.org/jira/browse/AURORA-626
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding a wait_for_batch_completion option into parallel updater.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/updater.py 
> 05b4c0c76ac2a8551f3aa370ab487f9f0802c3dc 
>   src/main/python/apache/aurora/client/api/updater_util.py 
> c5f8f23912701568e1ee6b69186a533fdd29a5d7 
>   src/main/python/apache/aurora/config/schema/base.py 
> 3b90ccb33e6ffef9e14befd42d95b8b8a94e949b 
>   src/test/python/apache/aurora/client/api/test_updater.py 
> 7020712c9f0b33ec29646482517768ccb13e881f 
> 
> Diff: https://reviews.apache.org/r/24126/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python:all
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 24126: Adding a wait_for_batch_completion option into parallel updater.

[Bill Farner]

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



src/main/python/apache/aurora/client/api/updater.py


Nit - this flag is only gated on "if not".  Consider inverting the meaning 
of the flag and don't negate.



src/main/python/apache/aurora/client/api/updater.py


Ditto - gate is always negative.  Consider inverting the meaning and return 
value of the function.



src/main/python/apache/aurora/client/api/updater.py


s/Wait/Waiting/



src/test/python/apache/aurora/client/api/test_updater.py


Can you add two more test cases with batch_size > 1:

 - instances % batch_size == 0
 - instances % batch_size != 0


- Bill Farner


On July 30, 2014, 11:59 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24126/
> ---
> 
> (Updated July 30, 2014, 11:59 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Bill Farner, and Brian Wickman.
> 
> 
> Bugs: AURORA-626
> https://issues.apache.org/jira/browse/AURORA-626
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding a wait_for_batch_completion option into parallel updater.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/updater.py 
> 05b4c0c76ac2a8551f3aa370ab487f9f0802c3dc 
>   src/main/python/apache/aurora/client/api/updater_util.py 
> c5f8f23912701568e1ee6b69186a533fdd29a5d7 
>   src/main/python/apache/aurora/config/schema/base.py 
> 3b90ccb33e6ffef9e14befd42d95b8b8a94e949b 
>   src/test/python/apache/aurora/client/api/test_updater.py 
> 7020712c9f0b33ec29646482517768ccb13e881f 
> 
> Diff: https://reviews.apache.org/r/24126/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python:all
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 24126: Adding a wait_for_batch_completion option into parallel updater.

[Maxim Khutornenko]

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

(Updated July 30, 2014, 11:59 p.m.)


Review request for Aurora, Kevin Sweeney, Bill Farner, and Brian Wickman.


Changes
---

+ wickman


Bugs: AURORA-626
https://issues.apache.org/jira/browse/AURORA-626


Repository: aurora


Description
---

Adding a wait_for_batch_completion option into parallel updater.


Diffs
-

  src/main/python/apache/aurora/client/api/updater.py 
05b4c0c76ac2a8551f3aa370ab487f9f0802c3dc 
  src/main/python/apache/aurora/client/api/updater_util.py 
c5f8f23912701568e1ee6b69186a533fdd29a5d7 
  src/main/python/apache/aurora/config/schema/base.py 
3b90ccb33e6ffef9e14befd42d95b8b8a94e949b 
  src/test/python/apache/aurora/client/api/test_updater.py 
7020712c9f0b33ec29646482517768ccb13e881f 

Diff: https://reviews.apache.org/r/24126/diff/


Testing
---

./pants src/test/python:all


Thanks,

Maxim Khutornenko

Review Request 24126: Adding a wait_for_batch_completion option into parallel updater.

[Maxim Khutornenko]

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

Review request for Aurora, Kevin Sweeney and Bill Farner.


Bugs: AURORA-626
https://issues.apache.org/jira/browse/AURORA-626


Repository: aurora


Description
---

Adding a wait_for_batch_completion option into parallel updater.


Diffs
-

  src/main/python/apache/aurora/client/api/updater.py 
05b4c0c76ac2a8551f3aa370ab487f9f0802c3dc 
  src/main/python/apache/aurora/client/api/updater_util.py 
c5f8f23912701568e1ee6b69186a533fdd29a5d7 
  src/main/python/apache/aurora/config/schema/base.py 
3b90ccb33e6ffef9e14befd42d95b8b8a94e949b 
  src/test/python/apache/aurora/client/api/test_updater.py 
7020712c9f0b33ec29646482517768ccb13e881f 

Diff: https://reviews.apache.org/r/24126/diff/


Testing
---

./pants src/test/python:all


Thanks,

Maxim Khutornenko