Change in vdsm[master]: jsonrpc: Stomp support

smizrahi at redhat.com smizrahi at redhat.com
Mon Jun 2 11:50:51 UTC 2014


Saggi Mizrahi has posted comments on this change.

Change subject: jsonrpc: Stomp support
......................................................................


Patch Set 16:

(11 comments)

http://gerrit.ovirt.org/#/c/26750/16/lib/yajsonrpc/__init__.py
File lib/yajsonrpc/__init__.py:

Line 560:     def _runRequest(self, ctx, request):
Line 561:         if self._threadFactory is None:
Line 562:             self._serveRequest(ctx, request)
Line 563:         else:
Line 564:             self._threadFactory(partial(self._serveRequest, ctx, request))
> Do we really want to use so naive thread pool implementation?
As you can see it's easy to put a more complex thread pool implementation.

Currently VDSM opens a thread per request for XMLRPC too so I leave it like this to preserve behavior. We can just pass a different scheme at a later time.

In short, we need to fix it but not here and not now.
Line 565: 
Line 566:     def stop(self):
Line 567:         self.log.info("Stopping JsonRPC Server")


http://gerrit.ovirt.org/#/c/26750/16/lib/yajsonrpc/stomp.py
File lib/yajsonrpc/stomp.py:

Line 13: _ESCAPE_CHARS = (('\\\\', '\\'), ('\\r', '\r'), ('\\n', '\n'), ('\\c', ':'))
Line 14: 
Line 15: 
Line 16: class Command:
Line 17:     MESSAGE = "MESSAGE"
> I think that we are missing few message types from the spec here.
Yap. But since we ignore them for now I don't see a need to add them
Line 18:     SEND = "SEND"
Line 19:     SUBSCRIBE = "SUBSCRIBE"
Line 20:     UNSUBSCRIBE = "UNSUBSCRIBE"
Line 21:     CONNECT = "CONNECT"


Line 72:     s = s.decode('utf-8')
Line 73:     for escaped, raw in _ESCAPE_CHARS:
Line 74:         s = s.replace(escaped, raw)
Line 75: 
Line 76:     # TODO : Throw error on invalid escape char
> It would be good idea to fix all the TODOs before merging.
Will fix.
Line 77:     # spec: Undefined escape sequences such as \t (octet 92 and 116) MUST be
Line 78:     # treated as a fatal protocol error.
Line 79:     return s
Line 80: 


Line 150:             self._contentLength = int(headers.get('content-length', -1))
Line 151:             self._state = self._STATE_BODY
Line 152:             return True
Line 153: 
Line 154:         # TODO: Proper error on missing or to many ':'
> the same as above
I'll change this to header.split(":", 1) and let decodeValue() throw an error in case of ":".
Line 155:         key, value = header.split(":")
Line 156:         key = decodeValue(key)
Line 157:         value = decodeValue(value)
Line 158: 


Line 282: 
Line 283:         self._aclient = AsyncClient(self, hostname)
Line 284:         adisp = self._adisp = AsyncDispatcher(self._aclient)
Line 285:         disp = self._disp = Dispatcher(adisp, sock, self._map)
Line 286:         sock.setblocking(True)
> Why do we need to set it to True for connect?
Because of SSL. If you set it to false SSL connection doesn't work. It's because the underlying SSLSocket doesn't work well with connection on nonblocking IO.
Line 287:         disp.connect(address)
Line 288:         sock.setblocking(False)
Line 289:         self.recv()  # wait for CONNECTED msg
Line 290: 


Line 363:     def handle_read(self, dispatcher):
Line 364:         pending = self._bufferSize
Line 365:         while pending:
Line 366:             try:
Line 367:                 data = dispatcher.recv(pending)
> I believe that we need to add here:
No because data could be none but pending() (line 373) has more data. since the buffer size we requested is too small.

I do make sure to only parse when data is None on line 380.
Line 368:             except socket.error:
Line 369:                 dispatcher.handle_error()
Line 370:                 return
Line 371: 


Line 414:         return True
Line 415: 
Line 416: 
Line 417: class AsyncClient(object):
Line 418:     log = logging.getLogger("yajsonrpc.protocols.stomp.AsyncClient")
> where us protocols package?
Oops. Leftover from refactoring. Will fix.
Line 419: 
Line 420:     def __init__(self, frameHandler, hostname):
Line 421:         self._hostname = hostname
Line 422:         self._frameHandler = frameHandler


Line 462:         if hasattr(frameHandler, "handle_message"):
Line 463:             frameHandler.handle_message(self, frame)
Line 464: 
Line 465:     def _process_receipt(self, frame, dispatcher):
Line 466:         # TODO:
> As above
I'm not going to implement receipt. I will put a better comment and a log.warning() in case someone actually expects a receipt.
Line 467:         pass
Line 468: 
Line 469:     def _process_error(self, frame, dispatcher):
Line 470:         raise StompError(frame)


http://gerrit.ovirt.org/#/c/26750/16/lib/yajsonrpc/stompReactor.py
File lib/yajsonrpc/stompReactor.py:

Line 27:     SSLDispatcher, \
Line 28:     SSLContext
Line 29: 
Line 30: 
Line 31: _Size = struct.Struct("!Q")
> Does this struct belongs to stomp. It was used as delimiter in json over tc
Leftovers, will remove
Line 32: 
Line 33: _STATE_LEN = "Waiting for message length"
Line 34: _STATE_MSG = "Waiting for message"
Line 35: 


Line 91:             self.log.warn("Unknown command %s", frame)
Line 92:             dispatcher.handle_error()
Line 93: 
Line 94: 
Line 95: class StompServer(object):
> StompServer and StompClient are very similar. Can we reuse code here?
I do reuse almost everything. Most common code is in StompAdapterImpl(). I'll try and get things more consolidated.
Line 96:     log = logging.getLogger("yajsonrpc.StompServer")
Line 97: 
Line 98:     def __init__(self, sock, reactor, addr, sslctx=None):
Line 99:         self._addr = addr


http://gerrit.ovirt.org/#/c/26750/16/tests/jsonRpcTests.py
File tests/jsonRpcTests.py:

Line 148:                          data[:plen], data[-plen:], len(data)))
Line 149: 
Line 150: 
Line 151: class _DummyBridge(object):
Line 152:     log = logging.getLogger("tests.DummyBridge")
> Do we need logging during tests?
Yes, it's good to know when messages arrive.

if you run the tests with -vvv you get that information inline.
Line 153: 
Line 154:     def echo(self, text):
Line 155:         self.log.info("ECHO: '%s'", text)
Line 156:         return text


-- 
To view, visit http://gerrit.ovirt.org/26750
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I22bcae1e150dea7bc7d9fecefb6847c48bfe8949
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Barak Azulay <bazulay at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list