Change in vdsm[master]: [WIP] Added glusterVolumeTop verb

Hello Ayal Baron, Bala.FA, Saggi Mizrahi, Federico Simoncelli, Dan Kenigsberg,

I'd like you to do a code review.  Please visit

http://gerrit.ovirt.org/7844

to review the following change.

Change subject: [WIP] Added glusterVolumeTop verb
......................................................................

[WIP] Added glusterVolumeTop verb

Added glusterVolumeTopOpen verb
Added glusterVolumeTopRead verb
Added glusterVolumeTopWrite verb
Added glusterVolumeTopOpenDir verb
Added glusterVolumeTopReadDir verb
Added glusterVolumeTopReadPerf
verb Added glusterVolumeTopWritePerf verb

Following is the output structure of glusterVolumeTopOpen
{'statusCode' : CODE,
    'brickCount': BRICK-COUNT,
    'bricks': {BRICK-NAME: {'count':FILE-COUNT,
                'currentOpenFds': CURRENT-OPEN-FDS-COUNT,
                'maxOpen': MAX-OPEN,
                'maxOpenTime': MAX-OPEN-TIME,
                'files': [{FILE-NAME: FILE-OPEN-COUNT}, ...]
               }, ...} }

Following is the output structure of glusterVolumeTopRead
{'statusCode': CODE,
        'brickCount': BRICK-COUNT,
        'topOp': TOP-OP,
        'bricks': {BRICK-NAME: {
                    'count': FILE-COUNT,
                    'files': [{FILE-NAME: FILE-READ-COUNT}, ...]}
                  ,...}}

Following is the output structure glusterVolumeTopWrite
{'statusCode' : CODE,
        'brickCount': BRICK-COUNT,
        'topOp': TOP-OP,
        'bricks': {BRICK-NAME: {'count': FILE-COUNT,
                     'files': [{FILE-NAME: FILE-WRITE-COUNT}...]}
                   ,...}}

Following is the output structure glusterVolumeTopOpenDir
{'statusCode': CODE,
        'brickCount': BRICK-COUNT,
        'topOp': TOP-OP,
        'bricks': {BRICK-NAME: {'count':OPEN-DIR-COUNT,
                      'files': [{DIR-NAME: DIR-OPEN-COUNT}, ...]}
                  ,...}

Following is the output structure glusterVolumeTopReadDir
{'statusCode': CODE,
        'brickCount': BRICK-COUNT,
        'topOp': TOP-OP,
        'bricks': {BRICK-NAME: {'count':READ-DIR-COUNT,
                      'files': [{DIR-NAME: DIR-READ-COUNT}, ...]}
                  ,...}

Following is the output structure glusterVolumeTopReadPerf
{'statusCode': CODE,
        'brickCount': BRICK-COUNT,
        'topOp': TOP-OP,
        'bricks': {BRICK-NAME: {'fileCount':READ-COUNT,
                     'throughput': BRICK-WISE-READ-THROUGHPUT,
                     ' timeTaken': TIME-TAKEN,
                     'files': [{FILE-NAME:
                       {'throughput':FILE-READ-THROUGHPUT,
                              'time': TIME}}, ...]}
                  ,...}}

Following is the output structure glusterVolumeTopWritePerf
{'statusCode': CODE,
        'brickCount': BRICK-COUNT,
        'topOp': TOP-OP,
        'bricks': {BRICK-NAME: {'fileCount':WRITE-COUNT,
                     'throughput': BRICK-WISE-WRITE-THROUGHPUT,
                     ' timeTaken': TIME-TAKEN,
                     'files': [{FILE-NAME:
                       {'throughput':FILE-WRITE-THROUGHPUT,
                             'time': TIME}}, ...]}
                  ,...}}

Change-Id: I96486363a9acb7472014a67fcd2d5185d4f3c428
Signed-off-by: Timothy Asir 
---
M vdsm/gluster/api.py
M vdsm/gluster/cli.py
M vdsm/gluster/exception.py
M vdsm_cli/vdsClientGluster.py
4 files changed, 372 insertions(+), 0 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/44/7844/1

diff --git a/vdsm/gluster/api.py b/vdsm/gluster/api.py
index e52430b..3f493e0 100644
--- a/vdsm/gluster/api.py
+++ b/vdsm/gluster/api.py
@@ -241,6 +241,61 @@
         status = self.svdsmProxy.glusterVolumeProfileInfo(volumeName)
         return {'profileInfo': status}

+    @exportAsVerb
+    def volumeTopOpen(self, volumeName, brickName=None, count=None,
+                      options=None):
+        status = self.svdsmProxy.glusterVolumeTopOpen(volumeName,
+                                                          brickName, count)
+        return {'topOpen': status}
+
+    @exportAsVerb
+    def volumeTopRead(self, volumeName, brickName=None, count=None,
+                      options=None):
+        status = self.svdsmProxy.glusterVolumeTopRead(volumeName,
+                                                          brickName, count)
+        return {'topRead': status}
+
+    @exportAsVerb
+    def volumeTopWrite(self, volumeName, brickName=None, count=None,
+                      options=None):
+        status = self.svdsmProxy.glusterVolumeTopWrite(volumeName,
+                                                          brickName, count)
+        return {'topWrite': status}
+
+    @exportAsVerb
+    def volumeTopOpenDir(self, volumeName, brickName=None, count=None,
+                      options=None):
+        status = self.svdsmProxy.glusterVolumeTopOpenDir(volumeName,
+                                                          brickName, count)
+        return {'topOpenDir': status}
+
+    @exportAsVerb
+    def volumeTopWriteDir(self, volumeName, brickName=None, count=None,
+                      options=None):
+        status = self.svdsmProxy.glusterVolumeTopWriteDir(volumeName,
+                                                          brickName, count)
+        return {'topWriteDir': status}
+
+    @exportAsVerb
+    def volumeTopReadPerf(self, volumeName, blockSize=None, count=None,
+                          brickName=None, listCount=None, options=None):
+        status = self.svdsmProxy.glusterVolumeTopReadPerf(volumeName,
+                                                          blockSize,
+                                                          count,
+                                                          brickName,
+                                                          listCount)
+        return {'topReadPerf': status}
+
+    @exportAsVerb
+    def volumeTopWritePerf(self, volumeName, blockSize=None, count=None,
+                          brickName=None, listCount=None, options=None):
+        status = self.svdsmProxy.glusterVolumeTopWritePerf(volumeName,
+                                                          blockSize,
+                                                          count,
+                                                          brickName,
+                                                          listCount)
+        return {'topWritePerf': status}
+

def getGlusterMethods(gluster):
     l = []
diff --git a/vdsm/gluster/cli.py b/vdsm/gluster/cli.py
index b91a04f..ba4768c 100644
--- a/vdsm/gluster/cli.py
+++ b/vdsm/gluster/cli.py
@@ -334,6 +334,66 @@
     return volumeInfoDict

+def _parseGlusterVolumeTopOpen(tree):
+    bricks = {}
+    for brick in tree.findall('volTop/brick'):
+        fileList = []
+        for file in brick.findall('file'):
+            fileList.append({file.find('filename').text:
+                             file.find('count').text})
+        bricks[brick.find('name').text] = {
+            'count': brick.find('members').text,
+            'currentOpen': brick.find('currentOpen').text,
+            'maxOpen': brick.find('maxOpen').text,
+            'maxOpenTime': brick.find('maxOpenTime').text,
+            'files': fileList}
+    status = {
+        'topOp': tree.find('volTop/topOp').text,
+        'brickCount': tree.find('volTop/brickCount').text,
+        'statusCode': tree.find('opRet').text,
+        'bricks': bricks}
+    return status
+
+
+def _parseGlusterVolumeTop(tree):
+    bricks = {}
+    for brick in tree.findall('volTop/brick'):
+        fileList = []
+        for fileTag in brick.findall('file'):
+            fileList.append({fileTag.find('filename').text:
+                             fileTag.find('count').text})
+        bricks[brick.find('name').text] = {
+            'count': brick.find('members').text,
+            'files': fileList}
+    status = {
+        'topOp': tree.find('volTop/topOp').text,
+        'brickCount': tree.find('volTop/brickCount').text,
+        'statusCode': tree.find('opRet').text,
+        'bricks': bricks}
+    return status
+
+
+def _parseGlusterVolumeTopPerf(tree):
+    bricks = {}
+    for brick in tree.findall('volTop/brick'):
+        fileList = []
+        for fileTag in brick.findall('file'):
+            fileList.append({fileTag.find('filename').text:
+                             {'count': fileTag.find('count').text,
+                             'time': fileTag.find('time').text}})
+        bricks[brick.find('name').text] = {
+            'count': brick.find('members').text,
+            'throughput': brick.find('throughput').text,
+            'timeTaken': brick.find('timeTaken').text,
+            'files': fileList}
+    status = {
+        'topOp': tree.find('volTop/topOp').text,
+        'brickCount': tree.find('volTop/brickCount').text,
+        'statusCode': tree.find("opRet").text,
+        'bricks': bricks}
+    return status
+
+
 def _parseGlusterVolumeProfileInfo(tree):
     bricks = {}
     for brick in tree.findall('volProfile/brick'):
@@ -819,3 +879,132 @@
         return _parseGlusterVolumeProfileInfo(xmltree)
     except:
         raise ge.GlusterXmlErrorException(err=out)
+
+
+@exportToSuperVdsm
+def volumeTopOpen(volumeName, brickName=None, count=None):
+    command = _getGlusterVolCmd() + ["top", volumeName, "open"]
+    if brickName:
+        command += ["brick", "%s" % brickName]
+    if count:
+        command += ["list-cnt", "%s" % count]
+    try:
+        xmltree, out = _execGlusterXml(command)
+    except ge.GlusterCmdFailedException, e:
+        raise ge.GlusterVolumeTopOpenFailedException(rc=e.rc, err=e.err)
+    try:
+        return _parseGlusterVolumeTopOpen(xmltree)
+    except:
+        raise ge.GlusterXmlErrorException(err=out)
+
+
+@exportToSuperVdsm
+def volumeTopRead(volumeName, brickName=None, count=None):
+    command = _getGlusterVolCmd() + ["top", volumeName, "read"]
+    if brickName:
+        command += ["brick", "%s" % brickName]
+    if count:
+        command += ["list-cnt", "%s" % count]
+    try:
+        xmltree, out = _execGlusterXml(command)
+    except ge.GlusterCmdFailedException, e:
+        raise ge.GlusterVolumeTopReadFailedException(rc=e.rc, err=e.err)
+    try:
+        return _parseGlusterVolumeTop(xmltree)
+    except:
+        raise ge.GlusterXmlErrorException(err=out)
+
+
+@exportToSuperVdsm
+def volumeTopWrite(volumeName, brickName=None, count=None):
+    command = _getGlusterVolCmd() + ["top", volumeName, "write"]
+    if brickName:
+        command += ["brick", "%s" % brickName]
+    if count:
+        command += ["list-cnt", "%s" % count]
+    try:
+        xmltree, out = _execGlusterXml(command)
+    except ge.GlusterCmdFailedException, e:
+        raise ge.GlusterVolumeTopWriteFailedException(rc=e.rc, err=e.err)
+    try:
+        return _parseGlusterVolumeTop(xmltree)
+    except:
+        raise ge.GlusterXmlErrorException(err=out)
+
+
+@exportToSuperVdsm
+def volumeTopOpenDir(volumeName, brickName=None, count=None):
+    command = _getGlusterVolCmd() + ["top", volumeName, "opendir"]
+    if brickName:
+        command += ["brick", "%s" % brickName]
+    if count:
+        command += ["list-cnt", "%s" % count]
+    try:
+        xmltree, out = _execGlusterXml(command)
+    except ge.GlusterCmdFailedException, e:
+        raise ge.GlusterVolumeTopOpenDirFailedException(rc=e.rc, err=e.err)
+    try:
+        return _parseGlusterVolumeTop(xmltree)
+    except:
+        raise ge.GlusterXmlErrorException(err=out)
+
+
+@exportToSuperVdsm
+def volumeTopReadDir(volumeName, bricName=None, count=None):
+    command = _getGlusterVolCmd() + ["top", volumeName, "write"]
+    if brickName:
+        command += ["brick", "%s" % brickName]
+    if count:
+        command += ["list-cnt", "%s" % count]
+    try:
+        xmltree, out = _execGlusterXml(command)
+    except ge.GlusterCmdFailedException, e:
+        raise ge.GlusterVolumeTopReadDirFailedException(rc=e.rc, err=e.err)
+    try:
+        return _parseGlusterVolumeTop(xmltree)
+    except:
+        raise ge.GlusterXmlErrorException(err=out)
+
+
+@exportToSuperVdsm
+def volumeTopReadPerf(volumeName, blockSize=None, count=None,
+                      brickName=None, listCount=None):
+    command = _getGlusterVolCmd() + ["top", volumeName, "read-perf"]
+    if blockSize:
+        command += ["bs", "%s" % blockSize]
+    if count:
+        command += ["count", "%s" % count]
+    if brickName:
+        command += ["brick", "%s" % brickName]
+    if listCount:
+        command += ["list-cnt", "%s" % listCount]
+    try:
+        xmltree, out = _execGlusterXml(command)
+    except ge.GlusterCmdFailedException, e:
+        raise ge.GlusterVolumeTopReadPerfFailedException(rc=e.rc, err=e.err)
+    try:
+        return _parseGlusterVolumeTopPerf(xmltree)
+    except:
+        raise ge.GlusterXmlErrorException(err=out)
+
+
+@exportToSuperVdsm
+def volumeTopWritePerf(volumeName, blockSize=None, count=None,
+                      brickName=None, listCount=None):
+    command = _getGlusterVolCmd() + ["top", volumeName, "write-perf"]
+    if blockSize:
+        command += ["bs", "%s" % blockSize]
+    if count:
+        command += ["count", "%s" % count]
+    if brickName:
+        command += ["brick", "%s" % brickName]
+    if listCount:
+        command += ["list-cnt", "%s" % listCount]
+    try:
+        xmltree, out = _execGlusterXml(command)
+    except ge.GlusterCmdFailedException, e:
+        raise ge.GlusterVolumeTopWritePerfFailedException(rc=e.rc, err=e.err)
+    try:
+        return _parseGlusterVolumeTopPerf(xmltree)
+    except:
+        raise ge.GlusterXmlErrorException(err=out)
diff --git a/vdsm/gluster/exception.py b/vdsm/gluster/exception.py
index bc20dd0..b392ec8 100644
--- a/vdsm/gluster/exception.py
+++ b/vdsm/gluster/exception.py
@@ -343,6 +343,41 @@
     message = "Volume profile info failed"

+class GlusterVolumeTopOpenFailedException(GlusterVolumeException):
+    code = 4161
+    message = "Volume top open failed"
+
+
+class GlusterVolumeTopReadFailedException(GlusterVolumeException):
+    code = 4162
+    message = "Volume top read failed"
+
+
+class GlusterVolumeTopWriteFailedException(GlusterVolumeException):
+    code = 4163
+    message = "Volume top write failed"
+
+
+class GlusterVolumeTopOpenDirFailedException(GlusterVolumeException):
+    code = 4164
+    message = "Volume top open dir failed"
+
+
+class GlusterVolumeTopReadDirFailedException(GlusterVolumeException):
+    code = 4165
+    message = "Volume top read dir failed"
+
+
+class GlusterVolumeTopReadPerfFailedException(GlusterVolumeException):
+    code = 4166
+    message = "Volume top read perf failed"
+
+
+class GlusterVolumeTopWritePerfFailedException(GlusterVolumeException):
+    code = 4167
+    message = "Volume top write perf failed"
+
+
 # Host
 class GlusterHostException(GlusterException):
     code = 4400
diff --git a/vdsm_cli/vdsClientGluster.py b/vdsm_cli/vdsClientGluster.py
index 8422695..3663c63 100644
--- a/vdsm_cli/vdsClientGluster.py
+++ b/vdsm_cli/vdsClientGluster.py
@@ -221,6 +221,41 @@
         pp.pprint(status)
         return status['status']['code'], status['status']['message']

+    def do_glusterVolumeTopOpen(self, args):
+        status = self.s.glusterVolumeTopOpen(args[0])
+        pp.pprint(status)
+        return status['status']['code'], status['status']['message']
+
+    def do_glusterVolumeTopRead(self, args):
+        status = self.s.glusterVolumeTopRead(args[0])
+        pp.pprint(status)
+        return status['status']['code'], status['status']['message']
+
+    def do_glusterVolumeTopWrite(self, args):
+        status = self.s.glusterVolumeTopWrite(args[0])
+        pp.pprint(status)
+        return status['status']['code'], status['status']['message']
+
+    def do_glusterVolumeTopOpenDir(self, args):
+        status = self.s.glusterVolumeTopOpenDir(args[0])
+        pp.pprint(status)
+        return status['status']['code'], status['status']['message']
+
+    def do_glusterVolumeTopReadDir(self, args):
+        status = self.s.glusterVolumeTopReadDir(args[0])
+        pp.pprint(status)
+        return status['status']['code'], status['status']['message']
+
+    def do_glusterVolumeTopReadPerf(self, args):
+        status = self.s.glusterVolumeTop(args[0])
+        pp.pprint(status)
+        return status['status']['code'], status['status']['message']
+
+    def do_glusterVolumeTopWritePerf(self, args):
+        status = self.s.glusterVolumeTop(args[0])
+        pp.pprint(status)
+        return status['status']['code'], status['status']['message']
+

def getGlusterCmdDict(serv):
     return {
@@ -403,4 +438,62 @@
              ('<volume_name>\n\t<volume_name> is existing volume name',
               'get gluster volume profile info'
               )),
+        'glusterVolumeTopOpen':
+            (serv.do_glusterVolumeTopOpen,
+             ('<volume_name> [brick=<existing_brick>] '
+              '[count=<list_count>]\n\t'
+              '<volume_name> is existing volume name\n\t'
+              'get volume top open fd count and maximum fd count of '
+              'a given volume with its all brick or specified brick'
+              )),
+        'glusterVolumeTopRead':
+            (serv.do_glusterVolumeTopRead,
+             ('<volume_name> [brick=<existing_brick>] '
+              '[count=<list_count>]\n\t'
+              '<volume_name> is existing volume name\n\t'
+              'get list of highest read calls on each brick or '
+              'a specified brick of a volume'
+              )),
+        'glusterVolumeTopWrite':
+            (serv.do_glusterVolumeTopWrite,
+             ('<volume_name> [brick=<existing_brick>] '
+              '[count=<list_count>]\n\t'
+              '<volume_name> is existing volume name\n\t'
+              'get list of highest write calls on each brick or '
+              'a specified brick of a volume'
+              )),
+        'glusterVolumeTopOpenDir':
+            (serv.do_glusterVolumeTopOpenDir,
+             ('<volume_name> [brick=<existing_brick>] '
+              '[count=<list_count>]\n\t'
+              '<volume_name> is existing volume name\n\t'
+              'get list of highest open calls on directories of each brick '
+              'or a specified brick of a volume'
+              )),
+        'glusterVolumeTopReadDir':
+            (serv.do_glusterVolumeTopReadDir,
+             ('<volume_name> [brick=<existing_brick>] '
+              '[count=<list_count>]\n\t'
+              '<volume_name> is existing volume name\n\t'
+              'get list of highest read calls on directories of each brick '
+              'or a specified brick of a volume'
+              )),
+        'glusterVolumeTopReadPerf':
+            (serv.do_glusterVolumeTopReadPerf,
+             ('<volume_name> [block_size=<block_size>] '
+              '[count=<count>] [brick=<existing_brick>] '
+              '[list_count=<list_count>]\n\t'
+              '<volume_name> is existing volume name\n\t'
+              'get list of read throughput of files on bricks. '
+              'if the block size and the count is not specified, '
+              'it will give the output based on historical data'
+              )),
+        'glusterVolumeTopWritePerf':
+            (serv.do_glusterVolumeTopWritePerf,
+             ('<volume_name> [block_size=<block_size>] '
+              '[count=<count>] [brick=<existing_brick>] '
+              '[list_count=<list_count>]\n\t'
+              '<volume_name> is existing volume name\n\t'
+              'get list of write throughput of files on bricks'
+              )),
         }

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I96486363a9acb7472014a67fcd2d5185d4f3c428
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Timothy Asir 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Bala.FA 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi

Change in vdsm[master]: Added glusterVolumeTop verb

Bala.FA has posted comments on this change.

Change subject: Added glusterVolumeTop verb
......................................................................

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

(2 inline comments)

....................................................
Commit Message
Line 3: AuthorDate: 2012-09-06 19:33:25 +0530
Line 4: Commit:     Timothy Asir 
Line 5: CommitDate: 2012-09-17 12:37:50 +0530
Line 6: 
Line 7: Added glusterVolumeTop verb
Add better commit message and explain each verb separately
Line 8: 
Line 9: Added glusterVolumeTopOpen verb
Line 10: Added glusterVolumeTopRead verb
Line 11: Added glusterVolumeTopWrite verb

....................................................
File vdsm/gluster/cli.py
Line 892:         raise ge.GlusterXmlErrorException(err=[etree.tostring(xmltree)])
Line 893: 
Line 894: 
Line 895: @exportToSuperVdsm
Line 896: def volumeTopOpen(volumeName, brickName=None, count=None):
Add docstring describing return value structure
Line 897:     command = _getGlusterVolCmd() + ["top", volumeName, "open"]
Line 898:     if brickName:
Line 899:         command += ["brick", "%s" % brickName]
Line 900:     if count:

--
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: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Timothy Asir 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Bala.FA 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Timothy Asir

Change in vdsm[master]: Added gluster volume top functional support

Timothy Asir has posted comments on this change.

Change subject: Added gluster volume top functional support
......................................................................

Patch Set 5: Verified

--
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: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Timothy Asir 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Bala.FA 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Timothy Asir

Change in vdsm[master]: Added gluster volume top functionalities

Timothy Asir has posted comments on this change.

Change subject: Added gluster volume top functionalities
......................................................................

Patch Set 6: Verified

--
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: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Timothy Asir 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Bala.FA 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Timothy Asir 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Added gluster volume top functionalities

Timothy Asir has posted comments on this change.

Change subject: Added gluster volume top functionalities
......................................................................

Patch Set 9: Verified

--
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: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Timothy Asir 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Bala.FA 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Timothy Asir 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Added gluster volume top functionalities

Bala.FA has posted comments on this change.

Change subject: Added gluster volume top functionalities
......................................................................

Patch Set 9: Looks good to me, but someone else must approve

--
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: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Timothy Asir 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Bala.FA 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Timothy Asir 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Added gluster volume top functionalities

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Added gluster volume top functionalities
......................................................................

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

(2 inline comments)

....................................................
Commit Message
Line 3: AuthorDate: 2012-09-06 19:33:25 +0530
Line 4: Commit:     Timothy Asir 
Line 5: CommitDate: 2012-12-05 18:00:21 +0530
Line 6: 
Line 7: Added gluster volume top functionalities
could you provide a short intro+refs for what is volumetop and its methods?
Line 8: 
Line 9: new verb: glusterVolumeTopOpen
Line 10: output structure:
Line 11:     When nfs is False:

....................................................
File vdsm/gluster/cli.py
Line 947:         raise ge.GlusterXmlErrorException(err=[etree.tostring(xmltree)])
Line 948: 
Line 949: 
Line 950: @exportToSuperVdsm
Line 951: def volumeTopOpen(volumeName, brickName=None, nfs=False, listCount=0):
what does brickName=None mean? all bricks? could you have both brickname and nfs=True?

I understand that you are mimicking gluster cli, but I'd prefer a more substantial documentation.
Line 952:     """
Line 953:     Returns:
Line 954:       When nfs=True:
Line 955:         [{'nfs': SERVER-NAME,

--
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: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Timothy Asir 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Bala.FA 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Timothy Asir 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Added gluster volume top functionalities

Timothy Asir has posted comments on this change.

Change subject: Added gluster volume top functionalities
......................................................................

Patch Set 9: No score

(2 inline comments)

....................................................
Commit Message
Line 3: AuthorDate: 2012-09-06 19:33:25 +0530
Line 4: Commit:     Timothy Asir 
Line 5: CommitDate: 2012-12-05 18:00:21 +0530
Line 6: 
Line 7: Added gluster volume top functionalities
Done
Line 8: 
Line 9: new verb: glusterVolumeTopOpen
Line 10: output structure:
Line 11:     When nfs is False:

....................................................
File vdsm/gluster/cli.py
Line 947:         raise ge.GlusterXmlErrorException(err=[etree.tostring(xmltree)])
Line 948: 
Line 949: 
Line 950: @exportToSuperVdsm
Line 951: def volumeTopOpen(volumeName, brickName=None, nfs=False, listCount=0):
If volume name only given, it provides output for all bricks (in this case, brickName=None refers all bricks when nfs is false)

if volume name and a specific brick name is given, it provides output for the specific brick

if volume name and nfs is given, it provides output for nfs only

and if volume name, brick name and nfs is given, the command execution will fail and throw an usage error.

is it good idea to have a two verbs; one is for brick and volume and another is for volume and nfs?
Line 952:     """
Line 953:     Returns:
Line 954:       When nfs=True:
Line 955:         [{'nfs': SERVER-NAME,

--
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: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Timothy Asir 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Bala.FA 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Timothy Asir 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Added gluster volume top functionalities

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Added gluster volume top functionalities
......................................................................

Patch Set 9: (1 inline comment)

....................................................
File vdsm/gluster/cli.py
Line 947:         raise ge.GlusterXmlErrorException(err=[etree.tostring(xmltree)])
Line 948: 
Line 949: 
Line 950: @exportToSuperVdsm
Line 951: def volumeTopOpen(volumeName, brickName=None, nfs=False, listCount=0):
no need to break all verbs apart and create nfs-specific ones. even though the semantics you describe is a bit convoluted.

I mostly lack documentation; I trust you guys to know much better about gluster api.
Line 952:     """
Line 953:     Returns:
Line 954:       When nfs=True:
Line 955:         [{'nfs': SERVER-NAME,

--
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: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Timothy Asir 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Bala.FA 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Timothy Asir 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Added gluster volume top functionalities

Timothy Asir has posted comments on this change.

Change subject: Added gluster volume top functionalities
......................................................................

Patch Set 9: (1 inline comment)

....................................................
File vdsm/gluster/cli.py
Line 947:         raise ge.GlusterXmlErrorException(err=[etree.tostring(xmltree)])
Line 948: 
Line 949: 
Line 950: @exportToSuperVdsm
Line 951: def volumeTopOpen(volumeName, brickName=None, nfs=False, listCount=0):
Done
Line 952:     """
Line 953:     Returns:
Line 954:       When nfs=True:
Line 955:         [{'nfs': SERVER-NAME,

--
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: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Timothy Asir 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Bala.FA 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Timothy Asir 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Gluster: Add gluster volume top functionality

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Gluster: Add gluster volume top functionality
......................................................................

Patch Set 10:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/323/ (2/3)

--
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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Bala.FA 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Timothy Asir 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Gluster: Add gluster volume top functionality

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Gluster: Add gluster volume top functionality
......................................................................

Patch Set 10:

Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/1177/ (1/3)

--
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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Bala.FA 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Timothy Asir 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Gluster: Add gluster volume top functionality

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Gluster: Add gluster volume top functionality
......................................................................

Patch Set 10:

Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/1212/ (3/3)

--
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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Bala.FA 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Timothy Asir 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Gluster: Add gluster volume top functionality

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Gluster: Add gluster volume top functionality
......................................................................

Patch Set 10:

Build Successful

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/1177/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/1212/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/323/ : SUCCESS

--
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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Bala.FA 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Timothy Asir 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Gluster: Add gluster volume top functionality

Timothy Asir has posted comments on this change.

Change subject: Gluster: Add gluster volume top functionality
......................................................................

Patch Set 10: Verified

--
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 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Bala.FA 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Timothy Asir 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Gluster: Add gluster volume top functionality

Aravinda VK has posted comments on this change.

Change subject: Gluster: Add gluster volume top functionality
......................................................................

Patch Set 10: (5 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}, ...]
We can have common format,

[{'nfs': Boolean, True if nfs else false
  'name': SERVER_NAME if nfs else BRICK_NAME
  'currentOpen': int,
  'files': [{FILE-NAME: {'count': int}}, ...],
  'maxOpen': int,
  'maxOpenTime': TIME}, ...]

OR

[{'type': 'nfs'
  'name': SERVER_NAME if nfs else BRICK_NAME
  'currentOpen': int,
  'files': [{FILE-NAME: {'count': int}}, ...],
  'maxOpen': int,
  'maxOpenTime': TIME}, ...]
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
can be simplified.

brick['nfs' if nfs else 'brick'] = name
Line 421: 
Line 422:         if mode == 'topopen':
Line 423:             brick['currentOpen'] = tag.find('currentOpen').text
Line 424:             brick['maxOpen'] = tag.find('maxOpen').text

Line 436: 
Line 437:         if mode == 'dir':
Line 438:             brick['dirs'] = fileList
Line 439:         else:
Line 440:             brick['files'] = fileList
Can be simplified. 
brick['dirs' if mode == 'dir' else 'files'] = fileList
Line 441:         bricks.append(brick)
Line 442:     return bricks
Line 443: 
Line 444:

Line 985:         command += ["brick", brickName]
Line 986:     if nfs:
Line 987:         command += ["nfs"]
Line 988:     if listCount:
Line 989:         command += ["list-cnt", "%s" % listCount]
Above command lines can be moved to separate function. Something like

def _getGlusterVolTopCmd(action, brickName=None, nfs=False, listCount=0, blockSize=0, count=0):
    command = _getGlusterVolCmd() + ["top", volumeName, action]
    command += ["brick", brickName] if brickName else []
    command += ["nfs"] if nfs else []
    command += ["list-cnt", "%s" % listCount] if listCount else []
    command += ["bs", "%s" % blockSize] if blockSize else []
    command += ["count", "%s" % count] if count else []

return command

This function can be called from different verbs.
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: 
We can move second exception into _parseVolumeTopGeneric function, so that try catch code in each verb can be reduced.

Btw, two try-except statement can be reduced to one.

Ex:

try:
    xmltree = _execGlusterXml(command)
    return _parseVolumeTopOpen(xmltree, nfs)
except ge.GlusterCmdFailedException, e:
    raise ge.GlusterVolumeTopOpenFailedException(rc=e.rc, err=e.err)
except (etree.ParseError, AttributeError, ValueError):
    raise ge.GlusterXmlErrorException(err=[etree.tostring(xmltree)])
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 
Gerrit-Reviewer: Aravinda VK 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Bala.FA 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Timothy Asir 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Gluster: Add gluster volume top functionality

Timothy Asir has posted comments on this change.

Change subject: Gluster: Add gluster volume top functionality
......................................................................

Patch Set 10: No score

(5 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}, ...]
currently gluster does not  support nfs for every verbs. That's why I kept it separately for few verbs.
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
I hope we can avoid any in-line if statement inside the statements for better readability.
Line 421: 
Line 422:         if mode == 'topopen':
Line 423:             brick['currentOpen'] = tag.find('currentOpen').text
Line 424:             brick['maxOpen'] = tag.find('maxOpen').text

Line 436: 
Line 437:         if mode == 'dir':
Line 438:             brick['dirs'] = fileList
Line 439:         else:
Line 440:             brick['files'] = fileList
as above!
Line 441:         bricks.append(brick)
Line 442:     return bricks
Line 443: 
Line 444:

Line 985:         command += ["brick", brickName]
Line 986:     if nfs:
Line 987:         command += ["nfs"]
Line 988:     if listCount:
Line 989:         command += ["list-cnt", "%s" % listCount]
as above!
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: 
As the _execGlusterXml function already catching ParseError, AttributeError, ValueError and raise an appropriate exception (with command execution failure code, error-message, etc); I hope, it is not a good idea to catch them commonly here; which may hides the actual error details!
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 
Gerrit-Reviewer: Aravinda VK 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Bala.FA 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Timothy Asir 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Gluster: Add gluster volume top functionality

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 
Gerrit-Reviewer: Aravinda VK 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Bala.FA 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Timothy Asir 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Gluster: Add gluster volume top functionality

Timothy Asir has posted comments on this change.

Change subject: Gluster: Add gluster volume top functionality
......................................................................

Patch Set 10: (3 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}, ...]
done
Line 29: 
Line 30: * glusterVolumeTopRead
Line 31:   output structure:
Line 32:     When nfs is False:

....................................................
File vdsm/gluster/cli.py
Line 985:         command += ["brick", brickName]
Line 986:     if nfs:
Line 987:         command += ["nfs"]
Line 988:     if listCount:
Line 989:         command += ["list-cnt", "%s" % listCount]
done
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: 
Yes I agree, this is valid when we have some common behaviour (under same process) in every statements. But here, the function _execGlusterXml internally calls an another (process) script (gluster cli) and combined the error/output with the usual python exception details. For this reason it's been placed inside the function itself.

I will move the second exception into _parseVolumeTopGeneric function and send a new patch.
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 
Gerrit-Reviewer: Aravinda VK 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Bala.FA 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Timothy Asir 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Gluster: Add gluster volume top functionality

Timothy Asir has posted comments on this change.

Change subject: Gluster: Add gluster volume top functionality
......................................................................

Patch Set 10: (1 inline comment)

....................................................
File vdsm/gluster/cli.py
Line 996:     except (etree.ParseError, AttributeError, ValueError):
Line 997:         raise ge.GlusterXmlErrorException(err=[etree.tostring(xmltree)])
Line 998: 
Line 999: 
Line 1000: @exportToSuperVdsm
I will change this name to @makePublic
Line 1001: def volumeTopRead(volumeName, brickName=None, nfs=False, listCount=0):
Line 1002:     """
Line 1003:     Returns top most files by their read count.
Line 1004:

--
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 
Gerrit-Reviewer: Aravinda VK 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Bala.FA 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Saggi Mizrahi 
Gerrit-Reviewer: Timothy Asir 
Gerrit-Reviewer: oVirt Jenkins CI Server

Change in vdsm[master]: Gluster: Add gluster volume top functionality

Sahina Bose has posted comments on this change.

Change subject: Gluster: Add gluster volume top functionality
......................................................................

Patch Set 10:

Tim, can you rebase this patch?

Change in vdsm[master]: Gluster: Add gluster volume top functionality

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Gluster: Add gluster volume top functionality
......................................................................

Patch Set 11:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/8805/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/8941/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/8015/ : SUCCESS

Change in vdsm[master]: Gluster: Add gluster volume top functionality

Itamar Heim has posted comments on this change.

Change subject: Gluster: Add gluster volume top functionality
......................................................................

Patch Set 11:

ping

Change in vdsm[master]: Gluster: Add gluster volume top functionality

[Jenkins CI RO]

Jenkins CI RO has abandoned this change.

Change subject: Gluster: Add gluster volume top functionality
......................................................................

Abandoned

Abandoned due to no activity - please restore if still relevant

Change in vdsm[master]: Gluster: Add gluster volume top functionality

[Jenkins CI RO]

Jenkins CI RO has posted comments on this change.

Change subject: Gluster: Add gluster volume top functionality
......................................................................

Patch Set 11:

Abandoned due to no activity - please restore if still relevant

Change in vdsm[master]: Gluster: Add gluster volume top functionality

[e-mail] has posted comments on this change.

Change subject: Gluster: Add gluster volume top functionality
......................................................................

Patch Set 11:

* Update tracker::IGNORE, no Bug-Url found