Change in vdsm[master]: jsonrpc: Implement the JsonRPC server for the next-gen API

smizrahi at redhat.com smizrahi at redhat.com
Wed Oct 17 20:10:23 UTC 2012


Saggi Mizrahi has posted comments on this change.

Change subject: jsonrpc: Implement the JsonRPC server for the next-gen API
......................................................................


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

(21 inline comments)

Sorry for being pedantic about variable names

cls VS className

In python, because you don't have types to hint you, you have to be very specific when choosing a variable name.

Also, using cls for something other then the current type object is as confusing as using self for something other then ... well... self.

It's as confusing as using i, and j for anything other then iteration counters in a loop (or worse, using n). These are just patterns people come to expect.

Make sure all you files pass pep8 (they currently don't).
New code should always conform to pep8. Just add the entire folder to the WHITELIST.
(just install syntastic and flake8 to have vim warn you about any violations)

Make sure parameter names are in mixedCase.

....................................................
File tests/apiData.py
Line 1: class APIData(object):
Copyright?
Line 2:     def __init__(self, obj, meth, data):
Line 3:         self.obj = obj
Line 4:         self.meth = meth
Line 5:         self.data = data


....................................................
File tests/apiTests.py
Line 131: 
Line 132:     schema = findSchema()
Line 133:     getFakeAPI()
Line 134:     import BindingJsonRpc
Line 135:     import Bridge
Why is that done here?
Line 136:     bridge = Bridge.DynamicBridge(schema)
Line 137:     server = BindingJsonRpc.BindingJsonRpc(bridge, ip, port)
Line 138:     server.start()
Line 139: 


....................................................
File vdsm_api/BindingJsonRpc.py
Line 21: import struct
Line 22: 
Line 23: _Size = struct.Struct("!Q")
Line 24: 
Line 25: __bridge__ = None
Globals are evil

Request handlers can access server attributes with the `server` field.

http://docs.python.org/library/socketserver.html#requesthandler-objects
Line 26: 
Line 27: class BindingJsonRpc:
Line 28:     def __init__(self, bridge, ip, port):
Line 29:         self.bridge = bridge


Line 28:     def __init__(self, bridge, ip, port):
Line 29:         self.bridge = bridge
Line 30:         self.serverPort = port
Line 31:         self.serverIP = ip
Line 32:         self.log = logging.getLogger('BindingJsonRpc')
Since every instance of the class gets the same logger you should make it a class attribute
Line 33:         self._createServer()
Line 34: 
Line 35:     def _createServer(self):
Line 36:         global __bridge__


Line 45:                         name='JsonRpc')
Line 46:         t.setDaemon(True)
Line 47:         t.start()
Line 48: 
Line 49:     def prepareForShutdown(self):
Why not just call it `shutdown`. That is what it does from the point of view of the class
Line 50:         self.server.shutdown()
Line 51: 
Line 52: 
Line 53: class JsonRpcServer(SocketServer.TCPServer):


Line 61:     The RequestHandler class for our server.
Line 62: 
Line 63:     It is instantiated once per connection to the server, and must
Line 64:     override the handle() method to implement communication to the
Line 65:     client.
I don't think you need to paste the class documentation here.
Line 66:     """
Line 67: 
Line 68:     def handle(self):
Line 69:         bridge = __bridge__


Line 66:     """
Line 67: 
Line 68:     def handle(self):
Line 69:         bridge = __bridge__
Line 70:         log = logging.getLogger('JsonRpcTCPHandler')
class attribute
Line 71:         while True:
Line 72:             # self.request is the TCP socket connected to the client
Line 73:             try:
Line 74:                 data = self.request.recv(_Size.size)


Line 79:                 msg = json.loads(self.request.recv(msgLen))
Line 80:             except:
Line 81:                 log.warn("Unexpected exception", exc_info=True)
Line 82:                 return
Line 83:             log.debug('--> %s', msg)
Not really a very descriptive log message
Line 84: 
Line 85:             try:
Line 86:                 ret = bridge._dispatch(msg['methodName'], msg.get('args', {}))
Line 87:             except Exception:


Line 87:             except Exception:
Line 88:                 log.error("Dispatch error", exc_info=True)
Line 89:                 continue
Line 90:             ret['id'] = msg['id']
Line 91:             msg = json.dumps(ret)
This can throw an exception.
Line 92:             msg = msg.encode('utf-8')
Line 93:             msize = _Size.pack(len(msg))
Line 94:             resp = msize + msg
Line 95:             log.debug('<-- %s', msg)


Line 91:             msg = json.dumps(ret)
Line 92:             msg = msg.encode('utf-8')
Line 93:             msize = _Size.pack(len(msg))
Line 94:             resp = msize + msg
Line 95:             log.debug('<-- %s', msg)
again
Line 96: 
Line 97:             self.wfile.write(resp)
Line 98:             self.wfile.flush()


....................................................
File vdsm_api/Bridge.py
Line 27: 
Line 28: class MethodBridge(object):
Line 29: 
Line 30:     def _dispatch(self, name, argobj):
Line 31:         method = name.replace('.', '_')
I would name it methodName as it's not a method
Line 32:         result = None
Line 33:         error = {'code': 0, 'message': 'Success'}
Line 34:         try:
Line 35:             fn = getattr(self, method)


Line 44:         except Exception, e:
Line 45:             error = {'code': 5, 'message': 'Internal error: %s' % e}
Line 46:         return {'result': result, 'error': error}
Line 47: 
Line 48:     def getArgs(self, argobj, arglist):
return tuple(argobj[arg] for arg in arglist if arg in argobj)
Line 49:         ret = ()
Line 50:         for arg in arglist:
Line 51:             if arg in argobj:
Line 52:                 ret += (argobj[arg],)


Line 59:             return response[member]
Line 60:         except KeyError:
Line 61:             raise VdsmError(5, "Response is missing '%s' member" % member)
Line 62: 
Line 63: class DynamicBridge(MethodBridge):
A lot of the methods look like they shouldn't be public
Line 64:     def __init__(self, schema):
Line 65:         self.parseSchema(schema)
Line 66: 
Line 67: 


Line 85: 
Line 86: 
Line 87:     def __getattr__(self, attr):
Line 88:         if attr in self.commands:
Line 89:             cls, name = attr.split('_')
cls is usually reserved as a version of 'self' but for class method.
Usually people use klass to avoid overriding a key word

anyway this is neither because it's a className.
Line 90:             return partial(self.dynamicMethod, cls, name)
Line 91:         else:
Line 92:             raise AttributeError
Line 93: 


Line 107:             pass
Line 108:         return getattr(API, cls)
Line 109: 
Line 110:     def typeFixup(self, sym_name, sym_type, obj):
Line 111:         logging.info("typeFixup: '%s': '%s'" % (sym_name, sym_type))
warning
Line 112:         isList = False
Line 113:         if type(sym_type) is list:
Line 114:             sym_type = sym_type[0]
Line 115:             isList = True


Line 109: 
Line 110:     def typeFixup(self, sym_name, sym_type, obj):
Line 111:         logging.info("typeFixup: '%s': '%s'" % (sym_name, sym_type))
Line 112:         isList = False
Line 113:         if type(sym_type) is list:
you should use
if isinstance(sym_type, list)

I also think the variable name should be changed to symTypeName

http://www.python.org/dev/peps/pep-0008/

Object type comparisons should always use isinstance() instead of comparing types directly.
Line 114:             sym_type = sym_type[0]
Line 115:             isList = True
Line 116:         if sym_name[0] == '*':
Line 117:             sym_name = sym_name[1:]


Line 127:             itemList = [obj]
Line 128: 
Line 129:         for item in itemList:
Line 130:             if sym_type in typefixups:
Line 131:                 logging.error("Fixing up type %s", sym_type)
warning

errors occur only when the system can't recover gracefully
Line 132:                 typefixups[sym_type](item)
Line 133:             for (k, v) in symbol.get('data', {}).items():
Line 134:                 if k[0] == '*':
Line 135:                     k = k[1:]


Line 130:             if sym_type in typefixups:
Line 131:                 logging.error("Fixing up type %s", sym_type)
Line 132:                 typefixups[sym_type](item)
Line 133:             for (k, v) in symbol.get('data', {}).items():
Line 134:                 if k[0] == '*':
This bit seems to repeat

Maybe extract to a method?
Line 135:                     k = k[1:]
Line 136:                 if k in item:
Line 137:                     self.typeFixup(k, v, item[k])
Line 138: 


Line 140:     def fixupArgs(self, cmd, args):
Line 141:         argInfo = zip(self.commands[cmd].get('data', {}).items(), args)
Line 142:         for typeInfo, val in argInfo:
Line 143:             argName, argType = typeInfo
Line 144:             if argType not in self.types:
Should this even happen?
Print warning?
Line 145:                 continue
Line 146:             self.typeFixup(argName, argType, val)
Line 147: 
Line 148:     def fixupRet(self, cmd, result):


Line 150:         if retType is not None:
Line 151:             self.typeFixup('return', retType, result)
Line 152:         return result
Line 153: 
Line 154:     def dynamicMethod(self, cls, name, argobj):
cls in python usually denotes current class, especially when put as a method parameter

in any case it's a className
Line 155:         cmd = '%s_%s' % (cls, name)
Line 156:         ctorArgObj = argobj.get('__obj__', {})
Line 157:         ctorargs = self.getArgs(ctorArgObj, self.getObjargList(cls))
Line 158:         args = self.getArgs(argobj, self.getArgList(cmd))


Line 174:             msg = result['status']['message']
Line 175:             return {'code': code, 'message': msg}
Line 176: 
Line 177:         retfield = command_info.get(cmd, {}).get('ret')
Line 178:         if type(retfield) == types.FunctionType:
again
isisntance()
Line 179:             ret = retfield(result)
Line 180:         else:
Line 181:             ret = self.getResult(result, retfield)
Line 182:         return self.fixupRet(cmd, ret)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Idae0faa80ffc6a5af002a8a7151aa40dc9a6673d
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <agl at us.ibm.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>


More information about the vdsm-patches mailing list