Re: 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/ --- (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.
--- 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.
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.
--- 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.
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.
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.
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
