Yaniv Bronhaim has posted comments on this change. Change subject: stomp: server side subscriptions ......................................................................
Patch Set 28: (8 comments) https://gerrit.ovirt.org/#/c/38451/28/lib/yajsonrpc/stomp.py File lib/yajsonrpc/stomp.py: Line 20: Line 21: from vdsm.utils import monotonic_time Line 22: import re Line 23: Line 24: # REQUIRED_FOR: engine-3.5 does it mean that when we stop supporting engine-3.5 this code should be removed? Line 25: LEGACY_SUB_ID_REQ = "/queue/_local/vdsm/requests" Line 26: LEGACY_SUB_ID_RES = "/queue/_local/vdsm/reponses" Line 27: Line 28: _RE_ESCAPE_SEQUENCE = re.compile(r"\\(.)") Line 292: Line 293: Line 294: class AsyncDispatcher(object): Line 295: log = logging.getLogger("stomp.AsyncDispatcher") Line 296: what about the comments? what is connection and frame_handler? if we would have stompFrameHandler and stompConnection it could be easy to understand the options.. but we don't Line 297: def __init__(self, connection, frame_handler, bufferSize=4096): Line 298: self._frame_handler = frame_handler Line 299: self.connection = connection Line 300: self._bufferSize = bufferSize https://gerrit.ovirt.org/#/c/38451/28/lib/yajsonrpc/stompreactor.py File lib/yajsonrpc/stompreactor.py: Line 56: Line 57: class StompAdapterImpl(object): Line 58: log = logging.getLogger("Broker.StompAdapter") Line 59: Line 60: def __init__(self, reactor, sub_map, req_dest): please doc what sub_map\_sub_dests and req_dest are built from. Line 61: self._reactor = reactor Line 62: self._outbox = deque() Line 63: self._sub_dests = sub_map Line 64: self._req_dest = req_dest Line 194: """ Line 195: try: Line 196: self._handle_destination(dispatcher, req_dest, json.loads(request)) Line 197: except Exception: Line 198: # let json server process issue what does it mean? what issues can occur? if the handleMessage should report on wrong request format (for example) so why do you json.load it before? what handle_destination should\can do? Line 199: pass Line 200: dispatcher.connection.handleMessage(request) Line 201: Line 202: def _handle_destination(self, dispatcher, req_dest, request): Line 196: self._handle_destination(dispatcher, req_dest, json.loads(request)) Line 197: except Exception: Line 198: # let json server process issue Line 199: pass Line 200: dispatcher.connection.handleMessage(request) if you pass exceptions you'll never understand why the req_dest[id] does not exist Line 201: Line 202: def _handle_destination(self, dispatcher, req_dest, request): Line 203: """ Line 204: We could receive single message or batch of messages. We need Line 206: """ Line 207: if isinstance(request, list): Line 208: map(functools.partial(self._handle_destination, dispatcher, Line 209: req_dest), Line 210: request) so request can be requests? interesting Line 211: return Line 212: Line 213: self._req_dest[request.get("id")] = req_dest Line 214: Line 280: resp = json.loads(message) Line 281: destination = self._req_dest[resp.get("id")] Line 282: del self._req_dest["id"] Line 283: except: Line 284: # we could have no reply-to please catch whatever you see that can be here - im quite concern when you do such freestyle handling - KeyError, json format error .. its not only the reply-to if I understand it currently Line 285: pass Line 286: Line 287: try: Line 288: connections = self._sub_map[destination] Line 351: def send(self, message, destination=None, headers=None): Line 352: if destination is None: Line 353: destination = _DEFAULT_REQUEST_DESTINATION Line 354: Line 355: self.log.debug("Sending request") do we need this debug print? Line 356: self._aclient.send( Line 357: destination, Line 358: message, Line 359: headers -- To view, visit https://gerrit.ovirt.org/38451 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1493070f2ba66ca9d39a6661876c82c4727cad62 Gerrit-PatchSet: 28 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Dima Kuznetsov <dkuzn...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <mskri...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: Yeela Kaplan <ykap...@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
