Nir Soffer has posted comments on this change.
Change subject: jsonrpc: introduce new client
......................................................................
Patch Set 10:
(5 comments)
Partial review.
https://gerrit.ovirt.org/#/c/64502/10/lib/vdsm/client.py
File lib/vdsm/client.py:
Line 92: "deviceid": "ece09190-8d83-4a4f-a67c-c0088c6f9259",
Line 93: "poolid": "00000001-0001-0001-0001-000000000311",
"device": "disk",
Line 94: "shared": "false", "propagateerrors":
"off", "optional": "false"}],
Line 95: "display": "qxl", "maxvcpus": "16",
"maxmemslots": 16},
Line 96: "vmid": "d7207614-38e3-43c4-b8f2-6086867d0a84"}))
This is too complex to be useful documentation. I think the previous example with
{"vmId": "xxxyyy"} is already a complex parameter and good enough.
If you think we need a more complex example select a call with simpler args, and format
the arguments nicely.
Line 97:
Line 98: ClientError will be raised in case of a protocol error:
Line 99:
Line 100: vdsm.client.ClientError: Error sending request: [Errno 111]
Line 94: "shared": "false", "propagateerrors":
"off", "optional": "false"}],
Line 95: "display": "qxl", "maxvcpus": "16",
"maxmemslots": 16},
Line 96: "vmid": "d7207614-38e3-43c4-b8f2-6086867d0a84"}))
Line 97:
Line 98: ClientError will be raised in case of a protocol error:
Lets make it clear that this means vdsm did not get the request.
Line 99:
Line 100: vdsm.client.ClientError: Error sending request: [Errno 111]
Line 101: Connection refused
Line 102:
Line 100: vdsm.client.ClientError: Error sending request: [Errno 111]
Line 101: Connection refused
Line 102:
Line 103: TimeoutError will be raised when preconfigured amount of time has passed with
Line 104: no response:
Lets make it clear that the request may be received by vdsm, but we did not get a response
within the specified timeout. The caller is responsible for checking the status of this
request with vdsm. An example is storage apis using the job ids, the caller can query the
job status with vdsm.
Line 105:
Line 106: vdsm.client.TimeoutError: timeout waiting for a response
Line 107:
Line 108: ServerError will be raised if the command encountered execution error:
Line 104: no response:
Line 105:
Line 106: vdsm.client.TimeoutError: timeout waiting for a response
Line 107:
Line 108: ServerError will be raised if the command encountered execution error:
Lets make it clear that ServerError means that the request was received by vdsm, and
executing the request in vdsm failed, returning an error.
Line 109:
Line 110: vdsm.client.ServerError: Vdsm request failed
Line 111: (code=4, message=Virtual machine already exists)
Line 112:
Line 224: try:
Line 225: self.close()
Line 226: except Exception:
Line 227: if t is None:
Line 228: raise
__exit__ should only raise exception if it was raised during exit. So
if t
I have written too many similar __exit__ lately, I think we should check if
the built-in contextlib.closing is already implementing this logic, and if not, maybe have
our own closing that does this error handling, so we can do:
cli = client.Client(...)
with utils.closing(cli):
Instead of implementing duplicating this logic everywhere.
--
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: 10
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: Yaniv Bronhaim <ybronhei(a)redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation(a)ovirt.org>
Gerrit-HasComments: Yes