Francesco Romani has posted comments on this change. Change subject: jsonrpcvdscli: create a client for vdsm with jsonrpc ......................................................................
Patch Set 5: Code-Review-1 (4 comments) mostly questions, -1 for visibility 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? 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 explicit. Let's just use the 'VM.migrationCreate' constant. 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 you please remind me why do we need an explicit method for migrationCreate? 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 explicit close(), maybe also automatically called in __del__ ? Line 73: Line 74: Line 75: def connect(cif, client_socket): Line 76: stompClient = cif.createStompClient(client_socket) -- 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 <[email protected]> Gerrit-Reviewer: Dima Kuznetsov <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: Yeela Kaplan <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
