Nir Soffer has posted comments on this change. Change subject: wip: jsonrpc: introduce new client ......................................................................
Patch Set 9: (2 comments) We also need tests for this. https://gerrit.ovirt.org/#/c/64502/9/lib/vdsm/client.py File lib/vdsm/client.py: Line 95: def __init__(self, client): Line 96: self._client = client Line 97: self.default_timeout = yajsonrpc.CALL_TIMEOUT Line 98: Line 99: def call(self, method, args={}, timeout=None): args is optional now (good), but you are using mutable default argument, meaning that all calls share the same args dict. If some call will modify the dict, the modification will effect the next calls. There no way to have default dict argument safely, the safe way to do this is: def call(self, method, args=None, ...): if args is None: args = {} We also need docstring explaining what is each argument. For example timeout seems to be the number of seconds to wait for response. Line 100: if timeout is None: Line 101: timeout = self.default_timeout Line 102: req = yajsonrpc.JsonRpcRequest(method, args, reqId=str(uuid.uuid4())) Line 103: try: Line 105: except EnvironmentError as e: Line 106: raise ClientError(e) Line 107: Line 108: if not responses: Line 109: raise ClientError("timeout waiting for a response") I think we need to introduce a Timeout error, otherwise the user cannot tell if the issue was a timeout or other client error. Line 110: Line 111: resp = responses[0] Line 112: if resp.error: Line 113: raise ServerError(resp.error['code'], resp.error['message']) -- To view, visit https://gerrit.ovirt.org/64502 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idd45d7e88bf2246beaf30550b12201917f32c354 Gerrit-PatchSet: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Irit Goihman <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Irit Goihman <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Sivák <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Simone Tiraboschi <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list -- [email protected] To unsubscribe send an email to [email protected]
