Change in vdsm[master]: gluster: Gluster UFO verbs

barumuga at redhat.com barumuga at redhat.com
Wed Feb 13 06:20:19 UTC 2013


Bala.FA has posted comments on this change.

Change subject: gluster: Gluster UFO verbs
......................................................................


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

(7 inline comments)

....................................................
File vdsm_cli/vdsClientGluster.py
Line 17: # Refer to the README and COPYING files for full details of the license
Line 18: #
Line 19: 
Line 20: import pprint as pp
Line 21: import json
leave a newline to separate user/system modules
Line 22: from vdsClient import service
Line 23: 
Line 24: 
Line 25: class GlusterService(service):


....................................................
File vdsm/gluster/api.py
Line 216:         status = self.svdsmProxy.glusterVolumeProfileInfo(volumeName, nfs)
Line 217:         return {'profileInfo': status}
Line 218: 
Line 219:     @exportAsVerb
Line 220:     def swiftConfigGet(self, serverType, section=None, configOption=None):
please add options=None for future use
Line 221:         return self.svdsmProxy.glusterSwiftConfigGet(serverType,
Line 222:                                                      section,
Line 223:                                                      configOption)
Line 224: 


Line 222:                                                      section,
Line 223:                                                      configOption)
Line 224: 
Line 225:     @exportAsVerb
Line 226:     def swiftConfigSet(self, serverType, configDict):
please add options=None for future use
Line 227:         self.svdsmProxy.glusterSwiftConfigSet(serverType,
Line 228:                                               configDict)
Line 229: 
Line 230: 


....................................................
File vdsm/gluster/cli.py
Line 19: #
Line 20: 
Line 21: import xml.etree.cElementTree as etree
Line 22: from functools import wraps
Line 23: import ConfigParser
please add new line to separate system/user modules
Line 24: from vdsm import utils
Line 25: from vdsm import netinfo
Line 26: import exception as ge
Line 27: from hostname import getHostNameFqdn, HostNameException


Line 910:     config_file = SWIFT_CONFIG_FILES[serverType]
Line 911:     try:
Line 912:         config.readfp(open(config_file))
Line 913:     except IOError:
Line 914:         raise ge.GlusterSwiftConfigOpenFailedException()
please pass actual error from IOError to GlusterSwiftConfigOpenFailedException().   In there, construct self.message appropriately.
Line 915: 
Line 916:     return config
Line 917: 
Line 918: 


Line 921:     try:
Line 922:         with open(config_file, 'wb') as configFile:
Line 923:             config.write(configFile)
Line 924:     except IOError:
Line 925:         raise ge.GlusterSwiftConfigWriteFailedException()
same here
Line 926: 
Line 927: 
Line 928: @exportToSuperVdsm
Line 929: def swiftConfigGet(serverType, section=None, configOption=None):


....................................................
File vdsm/gluster/exception.py
Line 407:     message = "Invalid section"
Line 408: 
Line 409:     def __init__(self, *args, **kwargs):
Line 410:         section = kwargs["sec"] if "sec" in kwargs else ""
Line 411:         self.message = "%s: Invalid Section" % section
We need a docstring describing what kwargs are accepted.  Moreover I feel kwargs doesn't convey the api usability.  I would like positional argumnet instead.  But its your call
Line 412: 
Line 413: 
Line 414: class GlusterSwiftConfigInvalidServerException(GlusterGeneralException):
Line 415:     code = 4411


--
To view, visit http://gerrit.ovirt.org/10864
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie966fb515275a0768f67cbbe2055a07002355327
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Aravinda VK <avishwan at redhat.com>
Gerrit-Reviewer: Aravinda VK <avishwan at redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Shireesh Anjal <sanjal at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list