Aravinda VK has posted comments on this change. Change subject: gluster: Added gluster hooks support ......................................................................
Patch Set 18: (3 inline comments) .................................................... File vdsm/gluster/hooks.py Line 29: from . import safeWrite Line 30: Line 31: _glusterHooksPath = '/var/lib/glusterd/hooks/1' Line 32: _mimeType = magic.open(magic.MIME_TYPE) Line 33: _mimeType.load() loading can be done outside for loop. and only check inside for loop as it is in existing code. My point was, when somebody does 'from gluster import hooks' these lines gets executed(during adding these modules to superVdsm.) As per my understanding loading ctypes takes more time to load, people prefer lazy loading as required. Line 34: Line 35: Line 36: class HookLevel: Line 37: PRE = 'PRE' Line 195: update=True, enable=False): Line 196: enabledFile, disabledFile = _getHookFileNames(glusterCmd, Line 197: hookLevel.lower(), hookName) Line 198: hookFound = os.path.exists(enabledFile) or os.path.exists(disabledFile) Line 199: if update: Done Line 200: if not hookFound: Line 201: raise ge.GlusterHookNotFoundException(glusterCmd, hookLevel, Line 202: hookName) Line 203: else: Line 243: @checkArgs Line 244: @exportToSuperVdsm Line 245: def hookRemove(glusterCmd, hookLevel, hookName): Line 246: hookFile = _getHookFile(glusterCmd, hookLevel.lower(), hookName) Line 247: try: If that is required we can identify file not found error by error number. For instance, hookRemove is called to remove a disabled file. System calls with existing script: 3 System calls with modified script: 2 If hookRemove is called to remove an enabled hook file. System calls with existing script: 2 System calls with modified script: 1 Consider a worst scenario, path.exists returns true for enabled hook, but at the same time some cron job in background was run and moved enabled hook file to disabled file. These are just my observation, "it's easier to ask for forgiveness than permission" :) http://stackoverflow.com/questions/1835756/using-try-vs-if-in-python Line 248: os.remove(hookFile) Line 249: return True Line 250: except OSError, e: Line 251: errMsg = "[Errno %s] %s: '%s'" % (e.errno, e.strerror, e.filename) -- To view, visit http://gerrit.ovirt.org/9671 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3918aa035d90967f1297dc7fadcf14b6a9385c45 Gerrit-PatchSet: 18 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Timothy Asir <[email protected]> Gerrit-Reviewer: Aravinda VK <[email protected]> Gerrit-Reviewer: Ayal Baron <[email protected]> Gerrit-Reviewer: Bala.FA <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Timothy Asir <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
