Francesco Romani has posted comments on this change. Change subject: stomp: client side subscription ......................................................................
Patch Set 16: (7 comments) initial review, I need (much) more time to properly understand this big and complex patch. https://gerrit.ovirt.org/#/c/36368/16//COMMIT_MSG Commit Message: Line 7: stomp: client side subscription Line 8: Line 9: In this patch we introduce concept of subscription for client Line 10: perspective. We move queuing functionality out of AsyncDispatcher to Line 11: frame_handler. New StompRpcClient class is repsonsible for sending typo: "repsonsible" vs "responsible" Line 12: subscriptions and ClientRpcTransportAdapter class adds 'reply-to' header Line 13: using subscriptions id. Line 14: Line 15: https://gerrit.ovirt.org/#/c/36368/16/lib/yajsonrpc/stomp.py File lib/yajsonrpc/stomp.py: Line 51: ERROR = "ERROR" Line 52: RECEIPT = "RECEIPT" Line 53: Line 54: Line 55: class Headers: even though for enumeration-like classes is no harm, we should always innherit from object until we switch to python 3. Line 56: CONTENT_LENGTH = "content-length" Line 57: CONTENT_TYPE = "content-type" Line 58: SUBSCRIPTION = "subscription" Line 59: DESTINATION = "destination" Line 505: return sub Line 506: Line 507: def unsubscribe(self, sub): Line 508: self.queue_frame(Frame(Command.UNSUBSCRIBE, Line 509: {"id": sub.id})) Who cleans self._subscriptions ? Line 510: Line 511: Line 512: class _Subscription(object): Line 513: https://gerrit.ovirt.org/#/c/36368/16/lib/yajsonrpc/stompReactor.py File lib/yajsonrpc/stompReactor.py: Line 97: cy = max(cy, 1000) Line 98: Line 99: # The server can send a heart-beat every cy ms and doesn't want Line 100: # to receive any heart-beat from the client. Line 101: resp.headers["heart-beat"] = "%d,0" % (cy,) minor nit: no need of use a tuple here for 'cy', this should work fine and save a few characters: resp.headers["heart-beat"] = "%d,0" % cy If you want to do this change, let's do in a (far) future different patch. Line 102: dispatcher.setHeartBeat(cy) Line 103: Line 104: self.queue_frame(resp) Line 105: self._reactor.wakeup() Line 127: self._socket = sock Line 128: self._reactor = reactor Line 129: self._messageHandler = None Line 130: Line 131: self._aclient = aclient maybe _async_client is a bit more explanatory here Line 132: adisp = self._adisp = stomp.AsyncDispatcher(aclient) Line 133: self._dispatcher = Dispatcher(adisp, sock=sock, map=reactor._map) Line 134: Line 135: def send_raw(self, msg): Line 198: return self._socket.getsockname()[0] Line 199: Line 200: Line 201: class StompClient(object): Line 202: log = logging.getLogger("jsonrpc.AsyncoreClient") for a future patch: this seems to get a stale logger now. Line 203: Line 204: def __init__(self, sock, reactor): Line 205: self._reactor = reactor Line 206: self._messageHandler = None Line 219: Line 220: def connect(self): Line 221: self._stompConn.connect() Line 222: Line 223: def handle_message(self, sub, frame): why renamed? moreover, it seems unused (also in the old code) Line 224: if self._messageHandler is not None: Line 225: self._messageHandler((self, frame.body)) Line 226: Line 227: def setMessageHandler(self, msgHandler): -- To view, visit https://gerrit.ovirt.org/36368 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8e43658f1cebd637ea3abf4396d388afa041ae71 Gerrit-PatchSet: 16 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: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: Yaniv Bronheim <ybron...@redhat.com> Gerrit-Reviewer: Yeela Kaplan <ykap...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
