Aravinda VK has posted comments on this change.
Change subject: Gluster: Add gluster volume top functionality
......................................................................
Patch Set 10: (4 inline comments)
....................................................
Commit Message
Line 24: [{'brick': BRICK-NAME,
Line 25: 'currentOpen': int,
Line 26: 'files': [{FILE-NAME: {'count': int}}, ...],
Line 27: 'maxOpen': int,
Line 28: 'maxOpenTime': TIME}, ...]
I know that gluster does not support nfs for every verbs. But I am writing here about
Output format consistency. If we don't have nfs then additional key will indicate that
as in example, But if we change the output format itself then it will be
difficult/inconsistent for consumers of this API(Front end/Ovirt) to handle for multiple
cases.
Line 29:
Line 30: * glusterVolumeTopRead
Line 31: output structure:
Line 32: When nfs is False:
....................................................
File vdsm/gluster/cli.py
Line 416: name = hostName
Line 417: if nfs:
Line 418: brick['nfs'] = name
Line 419: else:
Line 420: brick['brick'] = name
ok.
Line 421:
Line 422: if mode == 'topopen':
Line 423: brick['currentOpen'] = tag.find('currentOpen').text
Line 424: brick['maxOpen'] = tag.find('maxOpen').text
Line 985: command += ["brick", brickName]
Line 986: if nfs:
Line 987: command += ["nfs"]
Line 988: if listCount:
Line 989: command += ["list-cnt", "%s" % listCount]
Inline is just example here. the point is repeating logic in all functions, we can move
the entire logic to seperate function to avoid repetition.
Line 990: try:
Line 991: xmltree = _execGlusterXml(command)
Line 992: except ge.GlusterCmdFailedException, e:
Line 993: raise ge.GlusterVolumeTopOpenFailedException(rc=e.rc, err=e.err)
Line 994: try:
Line 995: return _parseVolumeTopOpen(xmltree, nfs)
Line 996: except (etree.ParseError, AttributeError, ValueError):
Line 997: raise ge.GlusterXmlErrorException(err=[etree.tostring(xmltree)])
Line 998:
Second exception statement in every function is just duplicate code statements. Let us say
if we need to handle one more exception similar to ValueError then you have to modify in
all places/functions. If we move reusable statements into a common place then maintaining
it in future is easy.
Line 999:
Line 1000: @exportToSuperVdsm
Line 1001: def volumeTopRead(volumeName, brickName=None, nfs=False, listCount=0):
Line 1002: """
--
To view, visit
http://gerrit.ovirt.org/7844
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I96486363a9acb7472014a67fcd2d5185d4f3c428
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Timothy Asir <tjeyasin(a)redhat.com>
Gerrit-Reviewer: Aravinda VK <avishwan(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
Gerrit-Reviewer: Timothy Asir <tjeyasin(a)redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server