Irit Goihman has posted comments on this change. Change subject: wip: jsonrpc: introduce new client ......................................................................
Patch Set 9: (10 comments) https://gerrit.ovirt.org/#/c/64502/8/lib/vdsm/client.py File lib/vdsm/client.py: Line 17: # Line 18: # Refer to the README and COPYING files for full details of the license Line 19: # Line 20: ''' Line 21: vdsm client > This is not a jsonrpc client (generic client for server using jsonrpc) but Done Line 22: Line 23: This is a simple client which uses jsonrpc protocol introduced on ovirt 3.5. Line 24: This client is not aware of the available methods and parameters. Line 25: Line 26: The user should consult the schema to construct the wanted command. Line 27: Line 28: The client is invoked with: Line 29: Line 30: cli = client.connect(host, port, ssl) > The common way to show code examples is to indent them with 4 spaces, the # Done Line 31: Line 32: For example: Line 33: Line 34: cli = client.connect('localhost', 54321, True) Line 36: Failure will result in ClientError exception. Line 37: Line 38: Invoking commands: Line 39: Line 40: cli.call('Host.getVMList') > Maybe we can make args optional to make it nicer to call methods without ar Done Line 41: Line 42: cli.call('VM.getStats', {'vmID': 'bc26bd11-ee3b-4a56-80d4-770f383a47b9'}) Line 43: Line 44: The call method expects method name and a dictionary (optional) containing all Line 44: The call method expects method name and a dictionary (optional) containing all Line 45: mandatory parameters, as they appear in the schema. Line 46: Line 47: Complex parameters are also supported and share the same behavior. Line 48: > We need to see an example result, and explain about getting the exact resul I will address this comment in the next change Line 49: ''' Line 50: Line 51: Line 52: from __future__ import absolute_import Line 47: Complex parameters are also supported and share the same behavior. Line 48: Line 49: ''' Line 50: Line 51: > One empty line here is enough. Done Line 52: from __future__ import absolute_import Line 53: Line 54: import uuid Line 55: Line 90: self.code = code Line 91: self.message = message Line 92: Line 93: Line 94: class _Client(object): > These should come after the public apis. Done Line 95: def __init__(self, client): Line 96: self._client = client Line 97: self.default_timeout = yajsonrpc.CALL_TIMEOUT Line 98: Line 110: Line 111: resp = responses[0] Line 112: if resp.error: Line 113: raise ServerError(resp.error['code'], resp.error['message']) Line 114: > Let document if we may get here multiple responses, when I see this code I same Line 115: return resp.result Line 116: Line 117: def close(self): Line 118: self._client.close() Line 113: raise ServerError(resp.error['code'], resp.error['message']) Line 114: Line 115: return resp.result Line 116: Line 117: def close(self): > Let keep empty line after raising/returning - the rest of the function is h Done Line 118: self._client.close() Line 119: Line 120: def __enter__(self): Line 121: return self Line 118: self._client.close() Line 119: Line 120: def __enter__(self): Line 121: return self Line 122: > Here is a good place for the context manager interface. Done Line 123: def __exit__(self, t, v, tb): Line 124: try: Line 125: self.close() Line 126: except Exception: Line 120: def __enter__(self): Line 121: return self Line 122: Line 123: def __exit__(self, t, v, tb): Line 124: try: > This and the exceptions are the only public apis, so they should be at the Done Line 125: self.close() Line 126: except Exception: Line 127: if t is None: -- 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 <igoih...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Irit Goihman <igoih...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Sivák <msi...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Simone Tiraboschi <stira...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
