Change in vdsm[master]: gluster: Temporary fix for supervdsm memory leak.
danken at redhat.com
danken at redhat.com
Thu Sep 25 10:49:17 UTC 2014
Dan Kenigsberg has posted comments on this change.
Change subject: gluster: Temporary fix for supervdsm memory leak.
......................................................................
Patch Set 3: Code-Review-1
(4 comments)
http://gerrit.ovirt.org/#/c/33312/3/lib/vdsm/constants.py.in
File lib/vdsm/constants.py.in:
Line 72: # Path definitions
Line 73: #
Line 74: P_LIBVIRT_VMCHANNELS = '/var/lib/libvirt/qemu/channels/'
Line 75: P_VDSM = '@VDSMDIR@/'
Line 76: P_VDSM_GLUSTER = '@VDSMDIR@/gluster/'
> I feel its better to be moved to gfapi.py
right. actually, os.path.realpath(__file__) may be used there.
Line 77: P_VDSM_RPC = '@VDSMDIR@/rpc/'
Line 78: P_VDSM_HOOKS = '@HOOKSDIR@/'
Line 79: P_VDSM_LIB = '@VDSMLIBDIR@/'
Line 80: P_VDSM_RUN = '@VDSMRUNDIR@/'
http://gerrit.ovirt.org/#/c/33312/3/vdsm/gluster/gfapi.py
File vdsm/gluster/gfapi.py:
Line 26:
Line 27: import exception as ge
Line 28: from vdsm import constants
Line 29: from vdsm import utils
Line 30: from __init__ import makePublic
> Please move extra imports before workaround function
It's sad that we have to use this
from __init__ import makePublic
trick to allow importing from a package. Please add a comment about it.
Line 31:
Line 32:
Line 33: GLUSTER_VOL_PROTOCAL = 'tcp'
Line 34: GLUSTER_VOL_HOST = 'localhost'
Line 189: try:
Line 190: res = volumeStatvfsGet(args.volume, args.host,
Line 191: args.port, args.protocol)
Line 192: except ge.GlusterLibgfapiException:
Line 193: print "Failed to retrive volume stats"
> Please print to stderr
or use sys.stderr.write() to make Python 3 geeks happy.
But more importantly, why is the special handling of GlusterLibgfapiException ? ANY exception that happens during volumeStatvfsGet means just that. I think we can throw away the whole try-block.
Line 194: sys.exit(1)
Line 195: print json.dumps({'f_blocks': res.f_blocks, 'f_bfree': res.f_bfree,
Line 196: 'f_bsize': res.f_bsize, 'f_frsize': res.f_frsize,
Line 197: 'f_bavail': res.f_bavail, 'f_files': res.f_files,
Line 191: args.port, args.protocol)
Line 192: except ge.GlusterLibgfapiException:
Line 193: print "Failed to retrive volume stats"
Line 194: sys.exit(1)
Line 195: print json.dumps({'f_blocks': res.f_blocks, 'f_bfree': res.f_bfree,
It would be more succinct to have
dict(zip(res._fields, res))
And there's no need to use "print". json.dump() is enough.
Line 196: 'f_bsize': res.f_bsize, 'f_frsize': res.f_frsize,
Line 197: 'f_bavail': res.f_bavail, 'f_files': res.f_files,
Line 198: 'f_ffree': res.f_ffree, 'f_favail': res.f_favail,
Line 199: 'f_flag': res.f_flag, 'f_namemax': res.f_namemax},
--
To view, visit http://gerrit.ovirt.org/33312
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7079426178ce47008d9a3b83635afce98536ca34
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Darshan N <dnarayan at redhat.com>
Gerrit-Reviewer: Bala.FA <barumuga at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Darshan N <dnarayan at redhat.com>
Gerrit-Reviewer: Sahina Bose <sabose at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list