Change in vdsm[master]: Added gluster cli support in supervdsm.

danken at redhat.com danken at redhat.com
Sat Apr 28 20:52:53 UTC 2012


Dan Kenigsberg has posted comments on this change.

Change subject: Added gluster cli support in supervdsm.
......................................................................


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

(10 inline comments)

sorry, i couldn't finish this review any quicker.

....................................................
File vdsm/constants.py.in
Line 89: EXT_GLUSTER_CLI = '@GLUSTER_CLI_PATH@'
Saggi would thank you if you put @GLUSTER_CLI_PATH@ in gluster_cli.py only.

the rest of Vdsm should not know of this executable, and gluster_cli.py should not care about the other sh*t in this file.

....................................................
File vdsm/gluster/gluster_cli.py
Line 31: brickCountRegEx = re.compile('(\d+) x (\d+) = (\d+)')
that's local-only, too. prepend with _, please.

Line 40:         out = out[1:]
del out[0]

here as well

Line 315:     if 'PAUSED' in out[0].strip().upper():
repeated recalculation of out[0].strip().upper()

Line 322:         return 'UNKNOWN', message
It would be better to define several BRICK_STATUS_XXX constants instead of returning arbitrary strings. Maybe

class BrickStatus:
  PAUSED = 'PAUSED'
  ....

this would let pyflakes and co to catch typos

but this all depends on how this function is going to be used, which I do not know...

Line 324:         return 'NA', message
isn't this an error? (I have no idea, but if it is, better raise an exception).

Line 458: def _parsePeerStatus(out, gHostName, gUuid):
I could really use a test case showing how this function behaves with a typical input.

Line 460:         out = out[1:]
del out[0]

would be quicker

....................................................
File vdsm.spec.in
Line 770: %files gluster
thanks!

....................................................
File vdsm/supervdsmServer.py
Line 90:         glusterVolumeInfo = \
as long as you do not have something automatic here, I do not see any benefit of this approach relative to your original. I expected to see here an iteration over the function names which you want to expose.

--
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: 5
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