Change in vdsm[master]: Using CrabRPC to communicate between vdsm and super vdsm
smizrahi at redhat.com
smizrahi at redhat.com
Wed Mar 13 18:15:31 UTC 2013
Saggi Mizrahi has posted comments on this change.
Change subject: Using CrabRPC to communicate between vdsm and super vdsm
......................................................................
Patch Set 6: I would prefer that you didn't submit this
(11 inline comments)
....................................................
File vdsm/storage/remoteFileHandler.py
Line 59: log = logging.getLogger("Storage.CrabRPCServer")
Line 60:
Line 61: def __init__(self, myRead, myWrite):
Line 62: self.myRead = myRead
Line 63: self.myWrite = myWrite
fdopen, opens a new fd so you can close myRead and myWrite at the end of this method.
Line 64: self.rfile = os.fdopen(myRead, "r")
Line 65: self.wfile = os.fdopen(myWrite, "wa")
Line 66: self.registeredFunctions = {}
Line 67: self.registeredModules = {}
Line 116: func = getattr(func, part)
Line 117:
Line 118: return func(*args, **kwargs)
Line 119:
Line 120: def close(self):
handle rfile and wfile and make sure you handle double close
Line 121: if self.myRead is not None:
Line 122: os.close(self.myRead)
Line 123: self.myRead = None
Line 124:
Line 199: raise
Line 200: except:
Line 201: self.log.error("Problem with handler",
Line 202: exc_info=True)
Line 203: raise
Why? Do you like error paths?
Line 204:
Line 205: res, err = pickle.loads(rawResponse)
Line 206: if err is not None:
Line 207: raise err
....................................................
File vdsm/supervdsmAPI.py
Line 65: return wrapper
Line 66:
Line 67:
Line 68: @logDecorator
Line 69: def exportToSuperVdsm(func):
This all mechanism causes a double dependency even though only implied.
You need to get the oop specific code out of remoteFileHandler.py and make the executable of remoteFileHandler as a parameter and have a similar boilerplate for supervdsm.
Line 70: @wraps(func)
Line 71: def wrapper(*args, **kwargs):
Line 72: return func(*args, **kwargs)
Line 73:
....................................................
File vdsm/supervdsm.py
Line 30: _g_singletonSupervdsmInstance = None
Line 31: _g_singletonSupervdsmInstance_lock = threading.Lock()
Line 32:
Line 33: vdsm_r = None
Line 34: vdsm_w = None
They don't need to be global
Line 35:
Line 36: DEFAULT_CALL_TIMEOUT = 10
Line 37:
Line 38:
Line 35:
Line 36: DEFAULT_CALL_TIMEOUT = 10
Line 37:
Line 38:
Line 39: def setPipe(r, w):
Doesn't need to be global
Line 40: global vdsm_r
Line 41: global vdsm_w
Line 42:
Line 43: if vdsm_r is not None or vdsm_w is not None:
Line 39: def setPipe(r, w):
Line 40: global vdsm_r
Line 41: global vdsm_w
Line 42:
Line 43: if vdsm_r is not None or vdsm_w is not None:
Can be removed once the methods isn't global anymore.
Line 44: raise \
Line 45: Exception('trying to set supervdsm pipe that already initialized')
Line 46: vdsm_r = r
Line 47: vdsm_w = w
Line 46: vdsm_r = r
Line 47: vdsm_w = w
Line 48:
Line 49:
Line 50: def closePipe():
Doesn't need to be global
Line 51: global vdsm_r
Line 52: global vdsm_w
Line 53: os.close(vdsm_r)
Line 54: os.close(vdsm_w)
Line 62: t.setDaemon(True)
Line 63: t.start()
Line 64:
Line 65:
Line 66: def _keepAliveThread(log):
Keep alive?
Line 67: global vdsm_r
Line 68: if vdsm_r is None:
Line 69: panic('Cannot monitor supervdsm')
Line 70: poller = select.poll()
Line 87: def __getattr__(self, name):
Line 88: self._funcName = name
Line 89: return self
Line 90:
Line 91: def __call__(self, *args, **kwargs):
What?
Line 92: try:
Line 93: return self.callCrabRPCFunction(DEFAULT_CALL_TIMEOUT,
Line 94: self._funcName, *args, **kwargs)
Line 95: except Exception as e:
....................................................
File vdsm/supervdsmServer.py
Line 33: svdsm_r = None
Line 34: svdsm_w = None
Line 35:
Line 36:
Line 37: class SupervdsmManager(CrabRPCServer):
No inheritance, ever
Line 38:
Line 39: def __init__(self, svdsm_r, svdsm_w):
Line 40: self.log = logging.getLogger("SuperVdsm")
Line 41: supervdsmAPI.log = self.log
--
To view, visit http://gerrit.ovirt.org/11910
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f1053e7d1264003fa265e44899d8b02f98bd68a
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Adam Litke <agl at us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Royce Lv <lvroyce at linux.vnet.ibm.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
More information about the vdsm-patches
mailing list