Change in vdsm[master]: stomp: parsing buffer refactoring

piotr.kliczewski at gmail.com piotr.kliczewski at gmail.com
Thu Mar 12 13:17:04 UTC 2015


Piotr Kliczewski has posted comments on this change.

Change subject: stomp: parsing buffer refactoring
......................................................................


Patch Set 1:

(4 comments)

https://gerrit.ovirt.org/#/c/38666/1/lib/yajsonrpc/stomp.py
File lib/yajsonrpc/stomp.py:

Line 165:     def _flush(self):
Line 166:         self._buffer = ""
Line 167: 
Line 168:     def _write_buffer(self, buff):
Line 169:         self._buffer += buff
> so we are going to append data to a string, which is usually bad practice i
The reason is not to use cStringIO. It seems that the performance of it is not needed since we we do not use so large strings.
Line 170: 
Line 171:     def _get_buffer(self):
Line 172:         return self._buffer
Line 173: 


Line 172:         return self._buffer
Line 173: 
Line 174:     def _handle_terminator(self, term):
Line 175:         res, _, rest = self._buffer.partition(term)
Line 176:         if not _:
> well, if we do logic on this field, then we should give a name to it.
Will use a name instead of '_'
Line 177:             return None
Line 178: 
Line 179:         self._buffer = rest
Line 180: 


Line 396: 
Line 397: class AsyncDispatcher(object):
Line 398:     log = logging.getLogger("stomp.AsyncDispatcher")
Line 399: 
Line 400:     def __init__(self, frameHandler, bufferSize=4096):
> ok, but why? Only for performance?
I think performance is the reason.
Line 401:         self._frameHandler = frameHandler
Line 402:         self._bufferSize = bufferSize
Line 403:         self._parser = Parser()
Line 404:         self._outbox = deque()


Line 573: 
Line 574:         return subscriptionID
Line 575: 
Line 576: 
Line 577: class Subscription(object):
> is this used elsewhere outside this module? If not, what about making it _p
Good idea. Even in my subscription changes this class is not used outside of this module.
Line 578:     def __init__(self, client, subid, ack):
Line 579:         self._ack = ack
Line 580:         self._subid = subid
Line 581:         self._client = client


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1b835e0d8a5ca20e67f0562955255ba1ad6ae9a1
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list