Yaniv Bronhaim has posted comments on this change. Change subject: jsonrpc: events ......................................................................
Patch Set 19: Code-Review-1 (6 comments) https://gerrit.ovirt.org/#/c/38069/19/lib/yajsonrpc/__init__.py File lib/yajsonrpc/__init__.py: Line 167: Line 168: return JsonRpcResponse(result, error, reqId) Line 169: Line 170: Line 171: class Notification(object): why don't you comment when you introduce new class. I have no idea where do you plan to use this new class and what its purpose. if only clientIF uses it why do you need it at all ? def notify could just compose the json directly with the event id and set the callback Line 172: log = logging.getLogger("jsonrpc.Notification") Line 173: Line 174: def __init__(self, event_id, cb): Line 175: self._event_id = event_id https://gerrit.ovirt.org/#/c/38069/19/lib/yajsonrpc/stomp.py File lib/yajsonrpc/stomp.py: Line 75: class StompError(RuntimeError): Line 76: def __init__(self, frame): Line 77: self.frame = frame Line 78: super(RuntimeError, self).__init__( Line 79: self.body, shouldn't it be frame.body? Line 80: ) Line 81: Line 82: Line 83: class _HeartBeatFrame(object): https://gerrit.ovirt.org/#/c/38069/19/lib/yajsonrpc/stompReactor.py File lib/yajsonrpc/stompReactor.py: Line 303 Line 304 Line 305 Line 306 Line 307 we don't close the connections anymore? please mention in the comment message what and why you remove things Line 273: Line 274: def send( Line 275: self, Line 276: message, Line 277: destination=_DEFAULT_RESPONSE_DESTINATION, have a comment that explains what is the default_response_destination when you don't specify destination and what "destination" can be. Line 278: ): Line 279: self.log.debug("Sending response") Line 280: try: Line 281: resp = json.loads(message) https://gerrit.ovirt.org/#/c/38069/19/vdsm/clientIF.py File vdsm/clientIF.py: Line 17 Line 18 Line 19 Line 20 Line 21 not related.. https://gerrit.ovirt.org/#/c/38069/19/vdsm/rpc/BindingJsonRpc.py File vdsm/rpc/BindingJsonRpc.py: Line 31: Line 32: def __init__(self, bridge): Line 33: self._server = JsonRpcServer(bridge, _simpleThreadFactory) Line 34: self._reactor = StompReactor() Line 35: self.startReactor(self._reactor) why do you pass it.. its the class parameter :? Line 36: Line 37: def add_socket(self, reactor, client_socket): Line 38: reactor.createListener(client_socket, self._onAccept) Line 39: -- To view, visit https://gerrit.ovirt.org/38069 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id27b5ca1773139932eb5cb16921d5abec4991c5e Gerrit-PatchSet: 19 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Saggi Mizrahi <smizr...@redhat.com> Gerrit-Reviewer: Dima Kuznetsov <dkuzn...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
