Change in vdsm[master]: lib: Revert and refine error handling in tmap()

nsoffer at redhat.com nsoffer at redhat.com
Thu Mar 26 10:04:05 UTC 2015


Nir Soffer has posted comments on this change.

Change subject: lib: Revert and refine error handling in tmap()
......................................................................


Patch Set 1:

(3 comments)

https://gerrit.ovirt.org/#/c/39211/1//COMMIT_MSG
Commit Message:

Line 52: 
Line 53:     succeeded = concurrent.tmap(func, values)
Line 54: 
Line 55:     if not all(succeeded):
Line 56:         ...
> We knew that... I'm not sure I understand why you changed your mind. checki
I did not like the code I had to write with the previous api. You can see that the next patches are nicer now because of this change.
Line 57: 
Line 58: We can ignore unexpected errors, since tmap() will log them and fail
Line 59: loudly. We can also minimize try except block for expected errors.
Line 60: 


https://gerrit.ovirt.org/#/c/39211/1/lib/vdsm/concurrent.py
File lib/vdsm/concurrent.py:

Line 66:         t.join()
Line 67: 
Line 68:     if error[0] is not None:
Line 69:         t, v, tb = error[0]
Line 70:         raise t, v, tb
> just raise error[0]. and if you raise, the caller doesn't need to check - i
No, raising error[0] will loose the original tracback. This code will show the original traceback on the caller thread, so you have more context when debugging this.

For example, if you run this in the shell, you will get the original traceback form the tmap worker thread in the main thread.

And the caller *does* need to check if not all(succeeded), because what I'm doing in this change is handling differently expected errors (e.g,  scsi scan failed) and unexpected errors (e.g, name error because of bug in the code).

tmap() will raise only if func raises (e.g. bug in the code), but will not raise if func had expected failure (there is some problem with the environment).

All this is explained in the commit message and in the function docstring.
Line 71: 


https://gerrit.ovirt.org/#/c/39211/1/tests/concurrentTests.py
File tests/concurrentTests.py:

Line 56:             raise Error(x)
Line 57:         try:
Line 58:             concurrent.tmap(func, (1, 2, 3))
Line 59:         except Error as e:
Line 60:             self.assertEqual(e.args, (3,))
> can't you just check assertRaises ? simpler ..
No, I want to check that the last error was raised, so I need the exception object. This is the simplest way to get it.
Line 61:         else:


-- 
To view, visit https://gerrit.ovirt.org/39211
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0154b28ff7822c63e77181bbbf444c712bd0c31e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list