Change in vdsm[master]: jsonrpcvdscli: create a client for vdsm with jsonrpc

ykaplan at redhat.com ykaplan at redhat.com
Sun May 3 15:38:53 UTC 2015


Yeela Kaplan has posted comments on this change.

Change subject: jsonrpcvdscli: create a client for vdsm with jsonrpc
......................................................................


Patch Set 5:

(5 comments)

https://gerrit.ovirt.org/#/c/39203/5/lib/vdsm/jsonrpcvdscli.py
File lib/vdsm/jsonrpcvdscli.py:

Line 44: 
Line 45:     def _callMethod(self, methodName, *args):
Line 46:         try:
Line 47:             method = _COMMAND_CONVERTER[methodName]
Line 48:         except AttributeError as e:
> I'd still expect KeyError from here. What am I missing?
You are right,
I misunderstood your previous comment.
Thanks
Line 49:             raise InvalidCall(methodName, args, e)
Line 50: 
Line 51:         req = JsonRpcRequest(method, args, reqId=str(uuid4()))
Line 52:         call = self._client.call_async(req)


Line 59: 
Line 60:         return resp.result
Line 61: 
Line 62:     def migrationCreate(self, params):
Line 63:         self._callMethod(_COMMAND_CONVERTER['migrationCreate'],
> no need to pass through _COMMAND_CONVERTER here, the mapping is fixed and e
We could... But I just think that it's better for it all to go through one place and not have different constants in different places.
Line 64:                          params['vmId'],
Line 65:                          params)
Line 66:         return {'status': {'code': 0}}
Line 67: 


Line 65:                          params)
Line 66:         return {'status': {'code': 0}}
Line 67: 
Line 68:     def __getattr__(self, methodName):
Line 69:         return partial(self._callMethod(methodName))
> I can't think any reason this could not work also for migrationCreate. Can 
With migrationcreate it expects the different return value...
which is in the shape of a dictionary like xmlrpc returns...
Line 70: 
Line 71:     def __del__(self):
Line 72:         self._client.close()
Line 73: 


Line 68:     def __getattr__(self, methodName):
Line 69:         return partial(self._callMethod(methodName))
Line 70: 
Line 71:     def __del__(self):
Line 72:         self._client.close()
> I becoming more and more skeptical of cleanup in __del__. What about an exp
I'm not sure I understand what you mean... 
can you elaborate?

The reason it's done this way is for the jsonrpc client to be closed as soon as there are no more users to it, which means the moment this object is deleted...
Line 73: 
Line 74: 
Line 75: def connect(cif, client_socket):
Line 76:     stompClient = cif.createStompClient(client_socket)


Line 71:     def __del__(self):
Line 72:         self._client.close()
Line 73: 
Line 74: 
Line 75: def connect(cif, client_socket):
> "cif" is part of vdsm server. requiring an instance thereof in a vdsm *clie
I need to get the stomp client from the reactor which is in cif,
do you have a better idea?
Line 76:     stompClient = cif.createStompClient(client_socket)
Line 77:     if stompClient is None:
Line 78:         raise Exception("Failed to create a stomp client")
Line 79: 


-- 
To view, visit https://gerrit.ovirt.org/39203
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9dbd70d28968db1305628281015f7b2379c8058
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Dima Kuznetsov <dkuznets at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list