Yaniv Bronhaim has posted comments on this change.
Change subject: jsonrpc: introduce new client
......................................................................
Patch Set 10:
(3 comments)
https://gerrit.ovirt.org/#/c/64502/10/lib/vdsm/client.py
File lib/vdsm/client.py:
Line 108: ServerError will be raised if the command encountered execution error:
Line 109:
Line 110: vdsm.client.ServerError: Vdsm request failed
Line 111: (code=4, message=Virtual machine already exists)
Line 112:
you repeat the errors description twice (here and in the call method). I guess that the
next update won't update both locations so better to have only one description
Line 113:
Line 114: Please note that if the client isn't a context manager, it should be
Line 115: closed at the end of the run:
Line 116:
Line 192: '''
Line 193: if args is None:
Line 194: args = {}
Line 195: if timeout is None:
Line 196: timeout = self.default_timeout
why not just have it as default values? like "def call(self, method, args={},
timeout=self.default_timeout): "
I didn't understand nir's comment about it. why a modification will effect next
calls?
Line 197: req = yajsonrpc.JsonRpcRequest(method, args, reqId=str(uuid.uuid4()))
Line 198: try:
Line 199: responses = self._client.call(req, timeout=timeout)
Line 200: except EnvironmentError as e:
Line 224: try:
Line 225: self.close()
Line 226: except Exception:
Line 227: if t is None:
Line 228: raise
why is this type check ?
--
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