Dan Kenigsberg has posted comments on this change.
Change subject: stomp: client side subscription ......................................................................
Patch Set 20:
(9 comments)
https://gerrit.ovirt.org/#/c/36368/20/lib/yajsonrpc/__init__.py File lib/yajsonrpc/__init__.py:
Line 275: Line 276: class JsonRpcClient(object): Line 277: def __init__(self, transport): Line 278: self.log = logging.getLogger("jsonrpc.JsonRpcClient") Line 279: transport.set_message_handler(self._handleMessage) can you place these pep8 renames in their own patch? Line 280: self._transport = transport Line 281: self._runningRequests = {} Line 282: self._lock = Lock() Line 283: self._eventcbs = []
Line 337: self.log.warning( Line 338: "Got an error from server without an ID (%s)", Line 339: resp.error, Line 340: ) Line 341: unrelated Line 342: ctx = self._runningRequests.pop(resp.id) Line 343: Line 344: ctx.addResponse(resp) Line 345:
https://gerrit.ovirt.org/#/c/36368/20/lib/yajsonrpc/stomp.py File lib/yajsonrpc/stomp.py:
Line 40: "\n": "\n", Line 41: } Line 42: Line 43: Line 44: class Command(object): new-style is good - but could you place these cleanups in a separate thread Line 45: MESSAGE = "MESSAGE" Line 46: SEND = "SEND" Line 47: SUBSCRIBE = "SUBSCRIBE" Line 48: UNSUBSCRIBE = "UNSUBSCRIBE"
Line 287: except IndexError: Line 288: return None Line 289: Line 290: Line 291: class Outbox(object): can you place this refactor in its own patch? Line 292: Line 293: def __init__(self): Line 294: self._outbox = deque() Line 295:
Line 402: return int(round(monotonic_time() * 1000)) Line 403: Line 404: Line 405: class AsyncClient(object): Line 406: log = logging.getLogger("yajsonrpc.stomp.AsyncClient") unrelated log name change Line 407: Line 408: def __init__(self): Line 409: self._connected = False Line 410: self._outbox = Outbox()
Line 516: self.queue_frame(Frame(Command.UNSUBSCRIBE, Line 517: {"id": sub.id})) Line 518: Line 519: Line 520: class _Subscription(object): could you introduce this newly-used class in a separate patch? Line 521: Line 522: def __init__(self, client, destination, subid, ack, message_handler): Line 523: self._ack = ack Line 524: self._subid = subid
https://gerrit.ovirt.org/#/c/36368/20/lib/yajsonrpc/stompReactor.py File lib/yajsonrpc/stompReactor.py:
Line 24: _STATE_LEN = "Waiting for message length" Line 25: _STATE_MSG = "Waiting for message" Line 26: Line 27: Line 28: _DEFAULT_RESPONSE_DESTINATIOM = "jms.topic.vdsm_legacy_responses" please move the change to the patch that handle 3.5, 3.6+, and non-engine requests. The introduction of "legacy mode" is unrelated to multiple subscriptions. Line 29: _DEFAULT_REQUEST_DESTINATION = "jms.topic.vdsm_legacy_requests" Line 30: Line 31: _FAKE_SUB_ID = "__vdsm_fake_broker__" Line 32:
Line 87: ) Line 88: else: Line 89: resp = stomp.Frame(stomp.Command.CONNECTED, {"version": "1.2"}) Line 90: cx, cy = parseHeartBeatHeader( Line 91: frame.headers.get(stomp.Headers.HEARTEBEAT, "0,0") even introduce Headers constants is simpler if left to its own patch. Line 92: ) Line 93: Line 94: # Make sure the heart-beat interval is sane Line 95: if cy != 0:
https://gerrit.ovirt.org/#/c/36368/20/tests/jsonRpcTests.py File tests/jsonRpcTests.py:
Line 84 Line 85 Line 86 Line 87 Line 88
yes. this is no longer needed.
Done