Bala.FA has posted comments on this change. Change subject: vdsm: Gluster UFO verbs ......................................................................
Patch Set 4: I would prefer that you didn't submit this (9 inline comments) Partial review .................................................... Commit Message Line 12: glusterGetServices: To get the status of all UFO services Line 13: (glusterd, smb, memcached, swift) Line 14: Line 15: glusterManageServices: To start/stop/restart UFO related Line 16: services(glusterd, smb, memcached, swift) These verbs are not UFO only and generic, I like to have these verbs as separate patch Line 17: Line 18: glusterGetSwiftConfig: Get swift config items Line 19: Line 20: glusterSetSwiftConfig: Set the config items Line 17: Line 18: glusterGetSwiftConfig: Get swift config items Line 19: Line 20: glusterSetSwiftConfig: Set the config items Line 21: Please change verb names like glusterServicesGet for uniformity with existing verbs. This is applicable to all related function/method names Line 22: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=850443 Line 23: Line 24: Change-Id: Ie966fb515275a0768f67cbbe2055a07002355327 Line 18: glusterGetSwiftConfig: Get swift config items Line 19: Line 20: glusterSetSwiftConfig: Set the config items Line 21: Line 22: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=850443 Please remove this bug-url which is not for ovirt Line 23: Line 24: Change-Id: Ie966fb515275a0768f67cbbe2055a07002355327 Line 19: Line 20: glusterSetSwiftConfig: Set the config items Line 21: Line 22: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=850443 Line 23: Add output structure of all getter verbs in commit message, docstring in functions/methods Line 24: Change-Id: Ie966fb515275a0768f67cbbe2055a07002355327 .................................................... File vdsm/gluster/cli.py Line 62: def _getGlusterServiceCmd(serviceName, action): Line 63: if serviceName == "swift": Line 64: return ["/usr/bin/swift-init", "main", action] Line 65: else: Line 66: return ["/sbin/service", serviceName, action] 1. use constants, or utils.CommandPath instead of hard-coded value for /sbin/service etc 2. its better to have RHEL/Fedora specifc code for sysvinit, upstart and systemd Line 67: Line 68: Line 69: def exportToSuperVdsm(func): Line 70: @wraps(func) Line 1010: :param servicesDict dict with serviceName as key and action as value Line 1011: Ex: {"glusterd": "start", "swift": "stop"} Line 1012: :returns {"value": True} Line 1013: """ Line 1014: for serviceName in serviceDict: Not sure about performing bulk operations. Manage one service and engine could call for other services repeatedly if needed Line 1015: action = serviceDict[serviceName] Line 1016: Line 1017: # Allowing to start any service can have side effects Line 1018: if serviceName not in SUPPORTED_GLUSTER_SERVICES: Line 1035: Line 1036: :param None Line 1037: :returns list of dict with PID and status details for each service Line 1038: Ex: {"value": [{"name": "glusterd", "status": "RUNNING", pid: "1027"},..]} Line 1039: """ above comment apply here too. all=False can be an option to getServices()? Line 1040: serviceStatus = [] Line 1041: Line 1042: for serviceName in SUPPORTED_GLUSTER_SERVICES: Line 1043: rc, out, err = _execGluster(_getGlusterServiceCmd(serviceName, Line 1068: try: Line 1069: config.readfp(open(config_file)) Line 1070: except IOError as e: Line 1071: raise ge.GetGlusterSwiftConfigFailedException(e.errno, Line 1072: [""], No need to send empties, instead err=[e.strerror] is clean Line 1073: [e.strerror]) Line 1074: configValues = {} Line 1075: Line 1076: # Since config.items or config.get for any section will include default .................................................... File vdsm/gluster/exception.py Line 413: Line 414: Line 415: class GetGlusterSwiftConfigFailedException(GlusterException): Line 416: code = 4412 Line 417: message = "Get Gluster Swift config failed" I feel these exceptions are gluster generic, not gluster host command specific -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Aravinda VK <[email protected]> Gerrit-Reviewer: Aravinda VK <[email protected]> Gerrit-Reviewer: Bala.FA <[email protected]> Gerrit-Reviewer: Shireesh Anjal <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
