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 <igoihman(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Irit Goihman <igoihman(a)redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Sivák <msivak(a)redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer(a)redhat.com>
Gerrit-Reviewer: Simone Tiraboschi <stirabos(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes