Nir Soffer has uploaded a new change for review.
Change subject: misc: Safer and simpler itmap ......................................................................
misc: Safer and simpler itmap
The previous code had few issues:
- It used unlimited number of threads by default. This may lead to creation of 100's of threads if you do not specify a value. - It used non-daemon threads, which could lead to unwanted delay during vdsm shutdown. - It tried to yield results before all arguments were handled. This could lead to unwanted delay in argument processing, if the caller would block processing the results. - It started one thread per value, even if maxthreads was smaller than number of values. - It was too complicated.
Changes:
- The caller must specify the maximum number of threads. - Use daemon threads - Queue all values before yielding results - Start up to maxthreads worker threads, each processing multiple values - Simplify the code - Add test for error handling
Change-Id: Iba6116ac4003702c8e921cebaf494491a6f9afaf Signed-off-by: Nir Soffer nsoffer@redhat.com --- M tests/miscTests.py M vdsm/storage/misc.py 2 files changed, 42 insertions(+), 42 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/19/39119/1
diff --git a/tests/miscTests.py b/tests/miscTests.py index 31f64fa..4b3e3c3 100644 --- a/tests/miscTests.py +++ b/tests/miscTests.py @@ -196,7 +196,7 @@ # outOfProcess operation + 1. it let us know that oop and itmap operate # properly with their limitations data = frozenset(range(oop.HELPERS_PER_DOMAIN + 1)) - ret = frozenset(misc.itmap(dummy, data, misc.UNLIMITED_THREADS)) + ret = frozenset(misc.itmap(dummy, data, len(data))) self.assertEquals(ret, data)
def testMoreThreadsThanArgs(self): @@ -207,6 +207,13 @@ data = 1 self.assertRaises(ValueError, misc.itmap(int, data, 0).next)
+ def testErrors(self): + err = Exception() + def dummy(arg): + raise err + data = [1, 2, 3] + self.assertEqual(list(misc.itmap(dummy, data, 4)), [err] * len(data)) +
class RotateFiles(TestCaseBase):
diff --git a/vdsm/storage/misc.py b/vdsm/storage/misc.py index eb484c7..463fd04 100644 --- a/vdsm/storage/misc.py +++ b/vdsm/storage/misc.py @@ -58,7 +58,6 @@ STR_UUID_SIZE = 36 UUID_HYPHENS = [8, 13, 18, 23] MEGA = 1 << 20 -UNLIMITED_THREADS = -1
log = logging.getLogger('Storage.Misc')
@@ -882,53 +881,47 @@ raise exception
-def itmap(func, iterable, maxthreads=UNLIMITED_THREADS): +def itmap(func, iterable, maxthreads): """ - Make an iterator that computes the function using - arguments from the iterable. It works similar to tmap - by running each operation in a different thread, this - causes the results not to return in any particular - order so it's good if you don't care about the order - of the results. - maxthreads stands for maximum threads that we can initiate simultaneosly. - If we reached to max threads the function waits for thread to - finish before initiate the next one. + Return an iterator calling func with arguments from iterable in multiple threads. + + Unlike tmap, the results are not returned in the original order of the + arguments, and number of threads is limited to maxthreads. """ - if maxthreads < 1 and maxthreads != UNLIMITED_THREADS: - raise ValueError("Wrong input to function itmap: %s", maxthreads) + if maxthreads < 1: + raise ValueError("Invalid maxthreads value: %s" % maxthreads)
- respQueue = Queue.Queue() + DONE = object() + values = Queue.Queue() + results = Queue.Queue()
- def wrapper(value): - try: - respQueue.put(func(value)) - except Exception as e: - respQueue.put(e) + def worker(): + while True: + value = values.get() + if value is DONE: + return + try: + results.put(func(value)) + except Exception as e: + results.put(e)
- threadsCount = 0 - for arg in iterable: - if maxthreads != UNLIMITED_THREADS: - if maxthreads == 0: - # This not supposed to happened. If it does, it's a bug. - # maxthreads should get to 0 only after threadsCount is - # greater than 1 - if threadsCount < 1: - raise RuntimeError("No thread initiated") - else: - yield respQueue.get() - # if yield returns one thread stopped, so we can run - # another thread in queue - maxthreads += 1 - threadsCount -= 1 + count = 0 + threads = 0
- t = threading.Thread(target=wrapper, args=(arg,)) - t.start() - threadsCount += 1 - maxthreads -= 1 + for value in iterable: + values.put(value) + count += 1 + if threads < maxthreads: + t = threading.Thread(target=worker) + t.daemon = True + t.start() + threads += 1
- # waiting for rest threads to end - for i in xrange(threadsCount): - yield respQueue.get() + for _ in range(threads): + values.put(DONE) + + for _ in xrange(count): + yield results.get()
def isAscii(s):
automation@ovirt.org has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 1:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
oVirt Jenkins CI Server has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 1:
Build Started (1/2) -> http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/17100/
Nir Soffer has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 1: Verified+1
automation@ovirt.org has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 2:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
oVirt Jenkins CI Server has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 2:
Build Started (1/2) -> http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/17101/
Nir Soffer has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 2: -Verified
With little more work, this version can replace concurrent.tmap(). The tests pass, but I want to do some sanity tests.
oVirt Jenkins CI Server has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 1:
Build Started (2/2) -> http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17274/
oVirt Jenkins CI Server has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 1: Code-Review-1 Verified-1
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/17100/ : UNSTABLE
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17274/ : FAILURE
oVirt Jenkins CI Server has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 2:
Build Started (2/2) -> http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17275/
oVirt Jenkins CI Server has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 2: Code-Review-1 Verified-1
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/17101/ : UNSTABLE
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17275/ : FAILURE
automation@ovirt.org has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 3:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
oVirt Jenkins CI Server has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 3:
Build Started (1/2) -> http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/17102/
oVirt Jenkins CI Server has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 3:
Build Started (2/2) -> http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17276/
oVirt Jenkins CI Server has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 3:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/17102/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17276/ : FAILURE
Allon Mureinik has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 3:
(3 comments)
https://gerrit.ovirt.org/#/c/39119/3//COMMIT_MSG Commit Message:
Line 8: Line 9: The previous code had few issues: Line 10: Line 11: - It used unlimited number of threads by default. This may lead to Line 12: creation of 100's of threads if you do not specify a value. s/100's/hundreds/, s/creation/the creation/ Line 13: - It used non-daemon threads, which could lead to unwanted delay during Line 14: vdsm shutdown. Line 15: - It tried to yield results before all arguments were handled. This Line 16: could lead to unwanted delay in argument processing, if the caller
Line 19: number of values. Line 20: - If invalid maxthreads was given, the ValueError was intialized with a Line 21: tuple ("Wrong input to function itmap: %s", maxthreads) instead of Line 22: a formatted string. Line 23: - It was too complicated. I'm not a fan of such vague statements. IMHO, either elaborate it or drop it. Line 24: Line 25: Changes: Line 26: Line 27: - The caller must specify the maximum number of threads.
https://gerrit.ovirt.org/#/c/39119/3/tests/miscTests.py File tests/miscTests.py:
Line 213: def dummy(arg): Line 214: raise err Line 215: Line 216: data = [1, 2, 3] Line 217: self.assertEqual(list(misc.itmap(dummy, data, 4)), [err] * len(data)) Just for the fun of it, I'd add a test with maxthreads = 2 (< len(data)). Not that I expect it to fail, but I think it has a nice declarative value to show you thought of this usecase and verified it works properly. Line 218: Line 219: Line 220: class RotateFiles(TestCaseBase): Line 221:
Nir Soffer has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 3:
(1 comment)
https://gerrit.ovirt.org/#/c/39119/3/tests/miscTests.py File tests/miscTests.py:
Line 213: def dummy(arg): Line 214: raise err Line 215: Line 216: data = [1, 2, 3] Line 217: self.assertEqual(list(misc.itmap(dummy, data, 4)), [err] * len(data))
Just for the fun of it, I'd add a test with maxthreads = 2 (< len(data)).
I will add permutations to test errors with len(data) -1, len(data), len(data) + 1 Line 218: Line 219: Line 220: class RotateFiles(TestCaseBase): Line 221:
Nir Soffer has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 3:
(2 comments)
https://gerrit.ovirt.org/#/c/39119/3//COMMIT_MSG Commit Message:
Line 8: Line 9: The previous code had few issues: Line 10: Line 11: - It used unlimited number of threads by default. This may lead to Line 12: creation of 100's of threads if you do not specify a value.
s/100's/hundreds/, s/creation/the creation/
Done Line 13: - It used non-daemon threads, which could lead to unwanted delay during Line 14: vdsm shutdown. Line 15: - It tried to yield results before all arguments were handled. This Line 16: could lead to unwanted delay in argument processing, if the caller
Line 19: number of values. Line 20: - If invalid maxthreads was given, the ValueError was intialized with a Line 21: tuple ("Wrong input to function itmap: %s", maxthreads) instead of Line 22: a formatted string. Line 23: - It was too complicated.
I'm not a fan of such vague statements. IMHO, either elaborate it or drop i
I will make it more clear - It was too hard for me to understand how the code worked. This is the main motivation of this patch, get the code into a state I fill confident to use it. Line 24: Line 25: Changes: Line 26: Line 27: - The caller must specify the maximum number of threads.
automation@ovirt.org has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 4:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
oVirt Jenkins CI Server has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 4:
Build Started (1/2) -> http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/17126/
automation@ovirt.org has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 5:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
oVirt Jenkins CI Server has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 5:
Build Started (1/2) -> http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/17127/
Nir Soffer has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 5:
Changes: - Improve commit message - Tests errors with different number of values and threads - Ensure logging of worker threads errors
automation@ovirt.org has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 6:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
oVirt Jenkins CI Server has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 6:
Build Started (1/2) -> http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/17129/
Nir Soffer has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 6:
This version rewrite the C-like loop queuing values and starting threads in more pythonic style, making it more clear how many threads are started.
Francesco Romani has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 6:
(2 comments)
nice improvement
https://gerrit.ovirt.org/#/c/39119/6/vdsm/storage/misc.py File vdsm/storage/misc.py:
Line 904: return Line 905: try: Line 906: results.put(func(value)) Line 907: except Exception as e: Line 908: results.put(e) nice! Line 909: Line 910: for value in iterable: Line 911: values.put(value) Line 912:
Line 922: a maybe you forgot an 'xrange' here (should be 'range')
Nir Soffer has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 6:
(1 comment)
https://gerrit.ovirt.org/#/c/39119/6/vdsm/storage/misc.py File vdsm/storage/misc.py:
Line 918: t.daemon = True Line 919: t.start() Line 920: values.put(DONE) Line 921: Line 922: for _ in xrange(count):
maybe you forgot an 'xrange' here (should be 'range')
count may be big, so we should use xrange on Python 2. Line 923: yield results.get() Line 924: Line 925: Line 926: def isAscii(s):
oVirt Jenkins CI Server has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 4:
Build Started (2/2) -> http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17300/
oVirt Jenkins CI Server has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 4:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/17126/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17300/ : FAILURE
oVirt Jenkins CI Server has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 5:
Build Started (2/2) -> http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17301/
oVirt Jenkins CI Server has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 5:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/17127/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17301/ : FAILURE
oVirt Jenkins CI Server has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 6:
Build Started (2/2) -> http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17303/
oVirt Jenkins CI Server has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 6:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/17129/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17303/ : FAILURE
Allon Mureinik has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 6: Code-Review+1
automation@ovirt.org has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 7:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Jenkins CI RO has abandoned this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Abandoned
Abandoned due to no activity - please restore if still relevant
gerrit-hooks has posted comments on this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 7:
* Update tracker: IGNORE, no Bug-Url found
Nir Soffer has restored this change.
Change subject: misc: Safer and simpler itmap ......................................................................
Restored
vdsm-patches@lists.fedorahosted.org