Change in vdsm[master]: stomp: server side subscriptions

ybronhei at redhat.com ybronhei at redhat.com
Wed Jun 3 06:50:51 UTC 2015


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.kliczewski at gmail.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Dima Kuznetsov <dkuznets at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan at redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourfali at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list