Change in vdsm[master]: stomp: outgoing connection to a broker

ybronhei at redhat.com ybronhei at redhat.com
Wed May 13 14:36:40 UTC 2015


Yaniv Bronhaim has posted comments on this change.

Change subject: stomp: outgoing connection to a broker
......................................................................


Patch Set 13: Code-Review-1

(9 comments)

https://gerrit.ovirt.org/#/c/38819/13/lib/vdsm/config.py.in
File lib/vdsm/config.py.in:

Line 180:             'Enable HTTP/1.1 keep-alive connections'),
Line 181: 
Line 182:         ('jsonrpc_enable', 'true', 'Enable the JSON RPC server'),
Line 183: 
Line 184:         ('broker_enable', 'false', 'Enable outgoing connection to broker'),
mention in the commit message what values you add to vdsm config and the reason for the defaults
Line 185: 
Line 186:         ('report_host_threads_as_cores', 'false',
Line 187:             'Count each cpu hyperthread as an individual core'),
Line 188: 


Line 323:             'Port where the broker is listening at.'),
Line 324: 
Line 325:         ('request_queues',
Line 326:             'jms.topic.vdsm_requests,jms.topic.vdsm_irs_requests',
Line 327:             'Queues to subscribe to for RPC requests'),
to or for?

what is jms?
Line 328: 
Line 329:         ('event_queue', 'jms.topic.vdsm_responses',
Line 330:             'Queue used for events'),
Line 331:     ]),


Line 326:             'jms.topic.vdsm_requests,jms.topic.vdsm_irs_requests',
Line 327:             'Queues to subscribe to for RPC requests'),
Line 328: 
Line 329:         ('event_queue', 'jms.topic.vdsm_responses',
Line 330:             'Queue used for events'),
jsonrpc events
Line 331:     ]),
Line 332: 
Line 333:     # Section: [sampling]
Line 334:     ('sampling', [


https://gerrit.ovirt.org/#/c/38819/13/lib/yajsonrpc/stompReactor.py
File lib/yajsonrpc/stompReactor.py:

Line 162:             return
Line 163:         elif stomp.LEGACY_SUB_ID_REQ == destination:
Line 164:             self._handle_internal(dispatcher, stomp.LEGACY_SUB_ID_RES,
Line 165:                                   frame.body)
Line 166:             return
no else? could it cause keyerror next line?
Line 167: 
Line 168:         subs = self._sub_dests[destination]
Line 169:         if not subs:
Line 170:             self._send_error("Subscription not available",


Line 256: 
Line 257: class StompServer(object):
Line 258:     log = logging.getLogger("yajsonrpc.StompServer")
Line 259: 
Line 260:     def __init__(self, reactor, subs):
why do you do all those words shortcuts? call it subscriptions please

req can easily refer to request, but when you also have res req sub its quite annoying in a code that should be readable imo.
Line 261:         self._reactor = reactor
Line 262:         self._messageHandler = None
Line 263:         self._sub_map = subs
Line 264:         self._req_dest = {}


Line 349: 
Line 350:     def send(
Line 351:         self,
Line 352:         message,
Line 353:         destination=stomp.LEGACY_SUB_ID_RES,
LEGACY_SUBSCRIPTION_ID_RESPONSE will fit in line
Line 354:         headers=None
Line 355:     ):
Line 356:         self.log.debug("Sending request")
Line 357:         self._aclient.send(


Line 436:         self.json_binding.add_socket(self._reactor, client_socket)
Line 437:         self.log.debug("Stomp detected from %s", socket_address)
Line 438: 
Line 439: 
Line 440: class ServerRpcContextAdapter(object):
please explain the need for this class . easy to understand but a comment should help readers.
Line 441:     @classmethod
Line 442:     def subscription_handler(cls, server):
Line 443:         def handler(sub, frame):
Line 444:             server.queueRequest(


https://gerrit.ovirt.org/#/c/38819/13/vdsm/clientIF.py
File vdsm/clientIF.py:

Line 91:         self._generationID = str(uuid.uuid4())
Line 92:         self.mom = None
Line 93:         self.bindings = {}
Line 94:         self._broker_client = None
Line 95:         self.subs = defaultdict(list)
self.subscriptions please
Line 96:         if _glusterEnabled:
Line 97:             self.gluster = gapi.GlusterApi(self, log)
Line 98:         else:
Line 99:             self.gluster = None


Line 194: 
Line 195:         self._acceptor = MultiProtocolAcceptor(self._reactor, host,
Line 196:                                                port, sslctx)
Line 197: 
Line 198:     def _connectToBroker(self):
who calls it?
Line 199:         if config.getboolean('vars', 'broker_enable'):
Line 200:             broker_address = config.get('addresses', 'broker_address')
Line 201:             broker_port = config.getint('addresses', 'broker_port')
Line 202:             request_queues = config.get('addresses', 'request_queues')


-- 
To view, visit https://gerrit.ovirt.org/38819
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ied3095a305fd5a5bfc19c8bc0342fba7cb811843
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Dima Kuznetsov <dkuznets at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.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