You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Maxim Khutornenko <ma...@apache.org> on 2014/07/31 01:57:39 UTC
Review Request 24126: Adding a wait_for_batch_completion option into
parallel updater.
-----------------------------------------------------------
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
Re: Review Request 24126: Adding a wait_for_batch_completion option into
parallel updater.
Posted by Mark Chu-Carroll <mc...@twopensource.com>.
-----------------------------------------------------------
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.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
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.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On July 31, 2014, 7 p.m., Mark Chu-Carroll wrote:
> > src/main/python/apache/aurora/client/api/updater.py, line 215
> > <https://reviews.apache.org/r/24126/diff/3/?file=646929#file646929line215>
> >
> > 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.
Posted by Mark Chu-Carroll <mc...@twopensource.com>.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/24126/#comment86208>
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.
Posted by Kevin Sweeney <ke...@apache.org>.
> On July 31, 2014, 12:04 p.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/client/api/updater.py, lines 234-235
> > <https://reviews.apache.org/r/24126/diff/3/?file=646929#file646929line234>
> >
> > 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.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On July 31, 2014, 7:04 p.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/client/api/updater.py, line 19
> > <https://reviews.apache.org/r/24126/diff/3/?file=646929#file646929line19>
> >
> > 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
> > <https://reviews.apache.org/r/24126/diff/3/?file=646929#file646929line234>
> >
> > 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.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/24126/#comment86200>
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
<https://reviews.apache.org/r/24126/#comment86210>
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.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
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.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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.
Posted by Bill Farner <wf...@apache.org>.
> On July 31, 2014, 12:19 a.m., Bill Farner wrote:
> > src/test/python/apache/aurora/client/api/test_updater.py, line 821
> > <https://reviews.apache.org/r/24126/diff/1/?file=646405#file646405line821>
> >
> > 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.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On July 31, 2014, 12:19 a.m., Bill Farner wrote:
> > src/test/python/apache/aurora/client/api/test_updater.py, line 821
> > <https://reviews.apache.org/r/24126/diff/1/?file=646405#file646405line821>
> >
> > 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.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On July 31, 2014, 12:19 a.m., Bill Farner wrote:
> > src/main/python/apache/aurora/client/api/updater.py, line 242
> > <https://reviews.apache.org/r/24126/diff/1/?file=646402#file646402line242>
> >
> > 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
> > <https://reviews.apache.org/r/24126/diff/1/?file=646402#file646402line245>
> >
> > 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
> > <https://reviews.apache.org/r/24126/diff/1/?file=646402#file646402line247>
> >
> > 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
> > <https://reviews.apache.org/r/24126/diff/1/?file=646405#file646405line821>
> >
> > 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.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On July 31, 2014, 12:19 a.m., Bill Farner wrote:
> > src/test/python/apache/aurora/client/api/test_updater.py, line 821
> > <https://reviews.apache.org/r/24126/diff/1/?file=646405#file646405line821>
> >
> > 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.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On July 31, 2014, 12:19 a.m., Bill Farner wrote:
> > src/test/python/apache/aurora/client/api/test_updater.py, line 821
> > <https://reviews.apache.org/r/24126/diff/1/?file=646405#file646405line821>
> >
> > 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.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
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
<https://reviews.apache.org/r/24126/#comment86043>
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
<https://reviews.apache.org/r/24126/#comment86044>
Ditto - gate is always negative. Consider inverting the meaning and return value of the function.
src/main/python/apache/aurora/client/api/updater.py
<https://reviews.apache.org/r/24126/#comment86045>
s/Wait/Waiting/
src/test/python/apache/aurora/client/api/test_updater.py
<https://reviews.apache.org/r/24126/#comment86050>
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.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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