Irit Goihman has posted comments on this change.
Change subject: wip: jsonrpc: introduce new client
......................................................................
Patch Set 6:
(17 comments)
https://gerrit.ovirt.org/#/c/64502/4/lib/vdsm/jsonrpcapi.py
File lib/vdsm/jsonrpcapi.py:
Line 15
Line 16
Line 17
Line 18
Line 19
How about vdsm.client?
Done
Line 19
Line 20
Line 21
Line 22
Line 23
Don't import functions, only modules. This make the dependencies
easier to
Done
Line 25
Line 26
Line 27
Line 28
Line 29
Better just to import yajsonrpc and use yajsonrpc.Xxx when needed.
Done
Line 28
Line 29
Line 30
Line 31
Line 32
Vdsm config may not be avilable.
Done
Line 31
Line 32
Line 33
Line 34
Line 35
This is a client, not a server.
Done
Line 34
Line 35
Line 36
Line 37
Line 38
You cannot use vdsm configuration here, the library may be used on
another
Done
Line 36
Line 37
Line 38
Line 39
Line 40
Please avoid using the schema here, we don't want to pay for
parsing the sc
Done
Line 44
Line 45
Line 46
Line 47
Line 48
We don't need setters like this in python. just make
default_timeout public
Done
Line 57
Line 58
Line 59
Line 60
Line 61
Can you explain why we need both *args and **kwargs? To send a
request we n
Done
Line 66
Line 67
Line 68
Line 69
Line 70
Please don't do that, no hidden arguments in kwargs. If you want
to support
Done
Line 70
Line 71
Line 72
Line 73
Line 74
Use the happy pass idiom:
Yes, it means we reached the timeout
and got no response. Usually it's because there is an error in the method
name/arguments or the basic case in which we lost connection to vdsm.
Line 73
Line 74
Line 75
Line 76
Line 77
You never want to raise Exception, the user of this api cannot do
anything
Done
Line 76
Line 77
Line 78
Line 79
Line 80
We don't need this.
Done
Line 82
Line 83
Line 84
Line 85
Line 86
Or this
Done
Line 88
Line 89
Line 90
Line 91
Line 92
Never use __del__.
Done
Line 96
Line 97
Line 98
Line 99
Line 100
Do not assume you have vdsm config here.
Done
Line 99
Line 100
Line 101
Line 102
Line 103
Same.
Done
--
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: 6
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