Piotr Kliczewski 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 re Yes 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 will add docstring 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. Done 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 repor Validation of the massage is done in different place. Here we are interested in destination only. 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 no Please see my comment above 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 We support bulk requests. 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 It should be KeyError when we get id. Will fix. 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? It is useful from client point of view. 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
