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