Change in vdsm[master]: Added gluster cli support in supervdsm.
danken at redhat.com
danken at redhat.com
Thu Apr 26 10:39:21 UTC 2012
Dan Kenigsberg has posted comments on this change.
Change subject: Added gluster cli support in supervdsm.
......................................................................
Patch Set 3: I would prefer that you didn't submit this
(11 inline comments)
....................................................
File vdsm/gluster_cli.py
Line 63: tokens = line.split(":", 1)
setting
key = tokens[0].strip().upper()
would save you some repeated code
Line 82: volumeInfo["brickCount"] = tokens[1].strip()
Engine loves string'ed integers, i do not. Do you?
Line 86: regex = re.compile('(\d+) x (\d+) = (\d+)')
why compile on every function call?
Line 445: hostName = ''
this should go into a separate function; consider using os.uname which is more reliably.h
Line 451: uuid = ''
yet another separate function, please.
for testability, hostName and uuid should be args of _parsePeerStatus()
Line 461: hostName = None
this is too obscure for me.
hostName starts as the name of the the running host. in a certain case it is reset to None, later to be set to a line's HOSTAME.
Maybe an example output of 'status' would make this clearer, maybe it should be rewritten.
Line 476: {HOSTNAME : {'uuid': UUID, 'hostName': HOSTNAME}, ...}
why repeat HOSTNAME in the keys, too?
Are you sure HOSTNAME is unique? uuid sounds more fitting as a key.
....................................................
File vdsm/gluster_exception.py
Line 366: raise NameError("Collision found: code %s is used by %s and %s" \
no need for backslash within parenthesis
....................................................
File vdsm.spec.in
Line 468: %{_datadir}/%{vdsm_name}/gluster_cli.py*
into vdsm-gluster.rpm, please?
....................................................
File vdsm/supervdsmServer.py
Line 31: from gluster_cli import volumeInfo as _glusterVolumeInfo
I think that would make more sense to import the module, once.
Line 106: def glusterVolumeInfo(*args, **kwargs):
seems like it would make sense to auto-generate this repetitions, maybe
setattr(_SuperVdsm, 'glusterVolumeInfo', logDecorator(lambda *args, **kwargs: getattr(gluster_cli, 'glusterVolumeInfo')))
would work (untested, I'm probably missing something)
--
To view, visit http://gerrit.ovirt.org/2797
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2faa261a3c44cf84af14102bdf6479287435793b
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Bala.FA <barumuga at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Ewoud Kohl van Wijngaarden <ewoud at kohlvanwijngaarden.nl>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin at redhat.com>
More information about the vdsm-patches
mailing list