Change in vdsm[master]: Make the event object per instance instead of per class

Saggi Mizrahi has uploaded a new change for review.

Change subject: Make the event object per instance instead of per class
......................................................................

Make the event object per instance instead of per class

Change-Id: Ia74e13d8cf719beba13f55b30501bda24aa8ade5
---
M vdsm/storage/securable.py
1 file changed, 10 insertions(+), 1 deletion(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/65/3165/1
--
To view, visit http://gerrit.ovirt.org/3165
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia74e13d8cf719beba13f55b30501bda24aa8ade5
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Make the event object per instance instead of per class
......................................................................

Patch Set 2: Looks good to me, but someone else must approve

(2 inline comments)

+1 because it fixes a nasty bug

....................................................
File vdsm/storage/securable.py
Line 41:         def _init(self, *args, **kwargs):
I'm not particularly thrilled to solve an easy problem using something complicated and then add something more complicated when an issue arises. Anyway I don't have time to fight.

Line 60:         origInit = fdict.get('__init__', None)
Why do you need origInit?

fdict['_o__init__'] = fdict.get('__init__', None)
 fdict['__init__'] = _init

--
To view, visit http://gerrit.ovirt.org/3165
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia74e13d8cf719beba13f55b30501bda24aa8ade5
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Make the event object per instance instead of per class
......................................................................

Patch Set 2: I would prefer that you didn't submit this

(2 inline comments)

-1 is for the commit message

....................................................
Commit Message
Line 7: Make the event object per instance instead of per class
please mention the BZ#, it makes the life of those building the product (me) much easier.

Luckily, we need securable.Securable only for sp.StoragePool - if two object would have needed to share the ._safety event, we'd been in deeper sh*t.

....................................................
File vdsm/storage/securable.py
Line 41:         def _init(self, *args, **kwargs):
Federico are you suggesting that we could have StoragePool inherit from Securable, or have it wrapped by a class modifying __init__?

--
To view, visit http://gerrit.ovirt.org/3165
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia74e13d8cf719beba13f55b30501bda24aa8ade5
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Make the event object per instance instead of per class
......................................................................

Patch Set 2: (1 inline comment)

....................................................
File vdsm/storage/securable.py
Line 41:         def _init(self, *args, **kwargs):
The only reason to use a metaclass (in this case) is that it's nice changing the class methods in the type. But, it's still something you could do when the object is instantiated (so a metaclass is not strictly required). In general metaclasses are useful if you want to modify the behavior of a type in a transparent way, but there's nothing transparent here (the class needs to call _setSafe, _setUnsafe, the methods must be marked as @unsercured, etc...), so it looks more like a subclassing pattern.
If want to keep the nice method-wrapping delivered by metaclasses we could have MySecureClass inherit from Securable which itself has a metaclass (idea stolen from Django :-).

--
To view, visit http://gerrit.ovirt.org/3165
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia74e13d8cf719beba13f55b30501bda24aa8ade5
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: Make the event object per instance instead of per class
......................................................................

Patch Set 2: I would prefer that you didn't submit this

(1 inline comment)

....................................................
File vdsm/storage/securable.py
Line 45:                 f(self, *args, **kwargs)
This is wrong, you shouldn't use self (it's already bound):

f(*args, **kwargs)

--
To view, visit http://gerrit.ovirt.org/3165
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia74e13d8cf719beba13f55b30501bda24aa8ade5
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Make the event object per instance instead of per class
......................................................................

Patch Set 3: I would prefer that you didn't submit this

Saggi, please abandon this, as we agreed on taking http://gerrit.ovirt.org/3198 instead.

--
To view, visit http://gerrit.ovirt.org/3165
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia74e13d8cf719beba13f55b30501bda24aa8ade5
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

Saggi Mizrahi has abandoned this change.

Change subject: Make the event object per instance instead of per class
......................................................................

Patch Set 3: Abandoned

Even though I still think this is the better solution

--
To view, visit http://gerrit.ovirt.org/3165
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: Ia74e13d8cf719beba13f55b30501bda24aa8ade5
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi