Nir Soffer has posted comments on this change.
Change subject: wip: jsonrpc: introduce new client
......................................................................
Patch Set 8:
(12 comments)
Much much nicer now.
Hopefully we get this this merge quickly, and the use this client to write the command
line tool.
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: jsonrpc client
This is not a jsonrpc client (generic client for server using jsonrpc) but client for
vdsm, so lets client vdsm client.
We can mention that this client uses the new jsonrpc protocol introduced in ovirt 3.5.
Line 22:
Line 23: This is a simple jsonrpc client which is not aware of the available
Line 24: 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 # prefix is not
needed.
This format is also markdown and restructedtext format for code block.
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 arguments?
cli.call('Host.getVMList')
cli.call("Host.getStats', {'vmID':
'bc26bd11-ee3b-4a56-80d4-770f383a47b9'})
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 containing all mandatory
Line 38: Invoking commands:
Line 39:
Line 40: # cli.call('Host.getVMList', {})
Line 41:
Line 42: # cli.call('VM.getStats',
{'vmID'='bc26bd11-ee3b-4a56-80d4-770f383a47b9'})
This is not dictionary syntax
Line 43:
Line 44: The call method expects method name and a dictionary containing all mandatory
Line 45: parameters, as they appear in the schema.
Line 46:
Line 43:
Line 44: The call method expects method name and a dictionary containing all mandatory
Line 45: parameters, as they appear in the schema.
Line 46:
Line 47: Complex parameters are also supported and share the same behavior.
This is not very clear, lets show an example request with complex parameters?
Line 48:
Line 49: '''
Line 50:
Line 51:
Line 44: The call method expects method name and a dictionary containing all mandatory
Line 45: 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 results as the
schema specifies.
Then we need to show how error are handled, both client errors, (connection refused?) and
server errors (no such vm?).
Finally we should mention that client must be closed, and show how to use it as context
manager:
with client.connect(..):
...
The documentation is actually the most important thing in this module, we want to make
easy to use, no surprises for users.
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.
Line 52: from __future__ import absolute_import
Line 53:
Line 54: import uuid
Line 55:
Line 90: try:
Line 91: self.close()
Line 92: except Exception:
Line 93: if t is None:
Line 94: raise
These should come after the public apis.
Line 95:
Line 96: def __init__(self, client):
Line 97: self._client = client
Line 98: self.default_timeout = yajsonrpc.CALL_TIMEOUT
Line 110:
Line 111: if not responses:
Line 112: raise ClientError("timeout waiting for a response")
Line 113:
Line 114: resp = responses[0]
Let document if we may get here multiple responses, when I see this code I wonder if we
never drop some responses...
If this should never return multiple responses, maybe add an assert here.
Line 115:
Line 116: if resp.error:
Line 117: raise ServerError(resp.error['code'],
resp.error['message'])
Line 118: return resp.result
Line 113:
Line 114: resp = responses[0]
Line 115:
Line 116: if resp.error:
Line 117: raise ServerError(resp.error['code'],
resp.error['message'])
Let keep empty line after raising/returning - the rest of the function is has lot of
whitespace, and on the last line we have no whitespace.
Line 118: return resp.result
Line 119:
Line 120: def close(self):
Line 121: self._client.close()
Line 118: return resp.result
Line 119:
Line 120: def close(self):
Line 121: self._client.close()
Line 122:
Here is a good place for the context manager interface.
Line 123:
Line 124: def connect(host, port, useSSL):
Line 125: try:
Line 126: client = stompreactor.SimpleClient(host, port, useSSL)
Line 120: def close(self):
Line 121: self._client.close()
Line 122:
Line 123:
Line 124: def connect(host, port, useSSL):
This and the exceptions are the only public apis, so they should be at the top of the
module.
Line 125: try:
Line 126: client = stompreactor.SimpleClient(host, port, useSSL)
Line 127: except Exception as e:
Line 128: raise ClientError(e)
--
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: 8
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