Saggi Mizrahi has uploaded a new change for review.
Change subject: jsonrpc: Optimize stomp encoder\decoder ......................................................................
jsonrpc: Optimize stomp encoder\decoder
Change-Id: I341e46463e654ae0a086cd49af18aa336b86cb6e Signed-off-by: Saggi Mizrahi smizrahi@redhat.com --- M lib/yajsonrpc/stomp.py 1 file changed, 60 insertions(+), 42 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/55/35255/1
diff --git a/lib/yajsonrpc/stomp.py b/lib/yajsonrpc/stomp.py index 884bff6..1dda6d7 100644 --- a/lib/yajsonrpc/stomp.py +++ b/lib/yajsonrpc/stomp.py @@ -14,8 +14,8 @@ # Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
import logging +import os import socket -import cStringIO from threading import Timer, Event from uuid import uuid4 from collections import deque @@ -27,6 +27,8 @@
_RE_ESCAPE_SEQUENCE = re.compile(r"\(.)")
+_RE_ENCODE_CHARS = re.compile(r"[\r\n\:]") + _EC_DECODE_MAP = { r"\": "\", r"r": "\r", @@ -34,7 +36,12 @@ r"c": ":", }
-_ESCAPE_CHARS = (('\\', '\'), ('\r', '\r'), ('\n', '\n'), ('\c', ':')) +_EC_ENCODE_MAP = { + ":": "\c", + "\": "\\", + "\r": "\r", + "\n": "\n", +}
class Command: @@ -70,6 +77,8 @@
class Frame(object): + __slots__ = ("headers", "command", "body") + def __init__(self, command="", headers=None, body=None): self.command = command if headers is None: @@ -87,15 +96,19 @@ if body is not None: self.headers["content-length"] = len(body)
- data = self.command + '\n' - data += '\n'.join(["%s:%s" % (encodeValue(key), encodeValue(value)) - for key, value in self.headers.iteritems()]) - data += '\n\n' - if body is not None: - data += body + data = [self.command, '\n'] + for key, value in self.headers.iteritems(): + data.append(encodeValue(key)) + data.append(":") + data.append(encodeValue(value)) + data.append("\n")
- data += "\0" - return data + data.append('\n') + if body is not None: + data.append(body) + + data.append("\0") + return ''.join(data)
def __repr__(self): return "<StompFrame command=%s>" % (repr(self.command)) @@ -111,24 +124,23 @@ raise ValueError("Contains illigal charachter `:`")
try: - return _RE_ESCAPE_SEQUENCE.sub( - lambda m: _EC_DECODE_MAP[m.groups()[0]], - s + s = _RE_ESCAPE_SEQUENCE.sub( + lambda m: _EC_DECODE_MAP[m.group(0)], + s, ) except KeyError as e: raise ValueError("Containes invalid escape squence `\%s`" % e.args[0])
+ return s.decode('utf-8') +
def encodeValue(s): - if not isinstance(s, (str, unicode)): - s = str(s) - for escaped, raw in _ESCAPE_CHARS: - s = s.replace(raw, escaped) - if isinstance(s, unicode): s = s.encode('utf-8') + elif not isinstance(s, str): + s = str(s)
- return s + return _RE_ENCODE_CHARS.sub(lambda m: _EC_ENCODE_MAP[m.group(0)], s)
class Parser(object): @@ -142,22 +154,29 @@ self._STATE_HEADER: self._parse_header, self._STATE_BODY: self._parse_body} self._frames = deque() - self._state = self._STATE_CMD + self._change_state(self._STATE_CMD) self._contentLength = -1 self._flush()
+ def _change_state(self, new_state): + self._state = new_state + self._state_cb = self._states[new_state] + def _flush(self): - self._buffer = cStringIO.StringIO() + self._buffer = "" + + def _write_buffer(self, buff): + self._buffer += buff + + def _get_buffer(self): + return self._buffer
def _handle_terminator(self, term): - if term not in self._buffer.getvalue(): + res, _, rest = self._buffer.partition(term) + if not _: return None
- data = self._buffer.getvalue() - res, rest = data.split(term, 1) - self._flush() - self._buffer.write(rest) - + self._buffer = rest return res
def _parse_command(self): @@ -174,7 +193,7 @@ cmd = decodeValue(cmd) self._tmpFrame = Frame(cmd)
- self._state = self._STATE_HEADER + self._change_state(self._STATE_HEADER) return True
def _parse_header(self): @@ -182,13 +201,13 @@ if header is None: return False
- headers = self._tmpFrame.headers if len(header) > 0 and header[-1] == '\r': header = header[:-1]
+ headers = self._tmpFrame.headers if header == "": self._contentLength = int(headers.get('content-length', -1)) - self._state = self._STATE_BODY + self._change_state(self._STATE_BODY) return True
key, value = header.split(":", 1) @@ -206,7 +225,7 @@
def _pushFrame(self): self._frames.append(self._tmpFrame) - self._state = self._STATE_CMD + self._change_state(self._STATE_CMD) self._tmpFrame = None self._contentLength = -1
@@ -226,16 +245,16 @@ return True
def _parse_body_length(self): - buf = self._buffer + buf = self._get_buffer() cl = self._contentLength - ndata = buf.tell() + ndata = len(buf) if ndata < cl: return False
remainingBytes = 0 self._flush() - body = buf.getvalue() - self._buffer.write(body[cl + 1:]) + body = buf + self._write_buffer(body[cl + 1:]) body = body[:cl]
if remainingBytes == 0: @@ -249,9 +268,8 @@ return len(self._frames)
def parse(self, data): - states = self._states - self._buffer.write(data) - while states[self._state](): + self._write_buffer(data) + while self._state_cb(): pass
def popFrame(self): @@ -347,7 +365,7 @@
def put_subscribe(self, destination, ack=None): subid = self._aclient.subscribe(self._adisp, destination, ack) - sub = Subsciption(self, subid, ack) + sub = Subscription(self, subid, ack) self._registerSubscription(sub) return sub
@@ -378,7 +396,7 @@ class AsyncDispatcher(object): log = logging.getLogger("stomp.AsyncDispatcher")
- def __init__(self, frameHandler, bufferSize=1024): + def __init__(self, frameHandler, bufferSize=4096): self._frameHandler = frameHandler self._bufferSize = bufferSize self._parser = Parser() @@ -471,7 +489,7 @@ return True
def _milis(self): - return int(round(time.time() * 1000)) + return int(os.times()[4] * 1000)
class AsyncClient(object): @@ -552,7 +570,7 @@ return subscriptionID
-class Subsciption(object): +class Subscription(object): def __init__(self, client, subid, ack): self._ack = ack self._subid = subid
oVirt Jenkins CI Server has posted comments on this change.
Change subject: jsonrpc: Optimize stomp encoder\decoder ......................................................................
Patch Set 1:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/12648/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/13600/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13438/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created_staging/217/ : FAILURE
Dima Kuznetsov has posted comments on this change.
Change subject: jsonrpc: Optimize stomp encoder\decoder ......................................................................
Patch Set 1:
(1 comment)
http://gerrit.ovirt.org/#/c/35255/1/lib/yajsonrpc/stomp.py File lib/yajsonrpc/stomp.py:
Line 96: if body is not None: Line 97: self.headers["content-length"] = len(body) Line 98: Line 99: data = [self.command, '\n'] Line 100: for key, value in self.headers.iteritems(): iteritems() is going away, maybe use items()? Line 101: data.append(encodeValue(key)) Line 102: data.append(":") Line 103: data.append(encodeValue(value)) Line 104: data.append("\n")
Yaniv Bronhaim has posted comments on this change.
Change subject: jsonrpc: Optimize stomp encoder\decoder ......................................................................
Patch Set 1:
(2 comments)
http://gerrit.ovirt.org/#/c/35255/1//COMMIT_MSG Commit Message:
Line 3: AuthorDate: 2014-11-18 00:55:35 +0200 Line 4: Commit: Saggi Mizrahi smizrahi@redhat.com Line 5: CommitDate: 2014-11-18 00:58:38 +0200 Line 6: Line 7: jsonrpc: Optimize stomp encoder\decoder i don't understand how it optimizes the code, more like fixing or correcting the code in most of the places Line 8: Line 9: Change-Id: I341e46463e654ae0a086cd49af18aa336b86cb6e
http://gerrit.ovirt.org/#/c/35255/1/lib/yajsonrpc/stomp.py File lib/yajsonrpc/stomp.py:
Line 76: _heartBeatFrame = _HeartBeatFrame() Line 77: Line 78: Line 79: class Frame(object): Line 80: __slots__ = ("headers", "command", "body") where do you use it ? Line 81: Line 82: def __init__(self, command="", headers=None, body=None): Line 83: self.command = command Line 84: if headers is None:
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Optimize stomp encoder\decoder ......................................................................
Patch Set 2:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/13946/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/13157/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/14110/ : FAILURE
Yaniv Bronhaim has posted comments on this change.
Change subject: stomp: Optimize stomp encoder\decoder ......................................................................
Patch Set 2:
(1 comment)
you don't reply to comments and than scream that we don't review it.. not nice..
http://gerrit.ovirt.org/#/c/35255/2/lib/yajsonrpc/stomp.py File lib/yajsonrpc/stomp.py:
Line 102: data.append(":") Line 103: data.append(encodeValue(value)) Line 104: data.append("\n") Line 105: Line 106: data.append('\n') why not \n\n? Line 107: if body is not None: Line 108: data.append(body) Line 109: Line 110: data.append("\0")
automation@ovirt.org has posted comments on this change.
Change subject: stomp: Optimize stomp encoder\decoder ......................................................................
Patch Set 3:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Optimize stomp encoder\decoder ......................................................................
Patch Set 3:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/15991/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/15190/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/16161/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created_staging/967/ : FAILURE
automation@ovirt.org has posted comments on this change.
Change subject: stomp: Optimize stomp encoder\decoder ......................................................................
Patch Set 4:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
automation@ovirt.org has posted comments on this change.
Change subject: stomp: Optimize stomp encoder\decoder ......................................................................
Patch Set 5:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Optimize stomp encoder\decoder ......................................................................
Patch Set 5:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/16349/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/15549/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/16519/ : FAILURE
automation@ovirt.org has posted comments on this change.
Change subject: stomp: Optimize stomp encoder\decoder ......................................................................
Patch Set 6:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Optimize stomp encoder\decoder ......................................................................
Patch Set 6:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/16461/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/16632/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_el_gerrit/15661/ : FAILURE
Francesco Romani has posted comments on this change.
Change subject: stomp: Optimize stomp encoder\decoder ......................................................................
Patch Set 6: Code-Review-1
(1 comment)
seems nice, but we need better documentation (commit message) to start
https://gerrit.ovirt.org/#/c/35255/6//COMMIT_MSG Commit Message:
Line 3: AuthorDate: 2014-11-18 00:55:35 +0200 Line 4: Commit: pkliczewski piotr.kliczewski@gmail.com Line 5: CommitDate: 2015-03-09 17:22:22 +0100 Line 6: Line 7: stomp: Optimize stomp encoder\decoder which is nice, but looks like this patch squashes together a few changes related only by the topic (optimization).
This commit message is way too terse. Can you please highlight and summarize the changes?
Maybe after that we can split this patch. Line 8: Line 9: Change-Id: I341e46463e654ae0a086cd49af18aa336b86cb6e
Piotr Kliczewski has posted comments on this change.
Change subject: stomp: Optimize stomp encoder\decoder ......................................................................
Patch Set 6:
(1 comment)
https://gerrit.ovirt.org/#/c/35255/6//COMMIT_MSG Commit Message:
Line 3: AuthorDate: 2014-11-18 00:55:35 +0200 Line 4: Commit: pkliczewski piotr.kliczewski@gmail.com Line 5: CommitDate: 2015-03-09 17:22:22 +0100 Line 6: Line 7: stomp: Optimize stomp encoder\decoder
which is nice, but looks like this patch squashes together a few changes re
Will explore splitting this patch and will update this message. Line 8: Line 9: Change-Id: I341e46463e654ae0a086cd49af18aa336b86cb6e
Piotr Kliczewski has posted comments on this change.
Change subject: stomp: Optimize stomp encoder\decoder ......................................................................
Patch Set 6:
(1 comment)
https://gerrit.ovirt.org/#/c/35255/6//COMMIT_MSG Commit Message:
Line 3: AuthorDate: 2014-11-18 00:55:35 +0200 Line 4: Commit: pkliczewski piotr.kliczewski@gmail.com Line 5: CommitDate: 2015-03-09 17:22:22 +0100 Line 6: Line 7: stomp: Optimize stomp encoder\decoder
Will explore splitting this patch and will update this message.
Splitting this patch in two so we have more functional based patches. Line 8: Line 9: Change-Id: I341e46463e654ae0a086cd49af18aa336b86cb6e
automation@ovirt.org has posted comments on this change.
Change subject: stomp: Optimize stomp encoder\decoder ......................................................................
Patch Set 7:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: stomp: Optimize stomp encoder\decoder ......................................................................
Patch Set 7: Verified+1
Verified by running engine 3.5, host installing vdsm and checking the logs for potential issues.
automation@ovirt.org has posted comments on this change.
Change subject: stomp: Optimize stomp encoder\decoder ......................................................................
Patch Set 8:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Optimize stomp encoder\decoder ......................................................................
Patch Set 8:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/16662/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/16832/ : FAILURE
Yaniv Bronhaim has posted comments on this change.
Change subject: stomp: Optimize stomp encoder\decoder ......................................................................
Patch Set 8: Code-Review+1
looks ok . please get more reviews on that
Francesco Romani has posted comments on this change.
Change subject: stomp: Optimize stomp encoder\decoder ......................................................................
Patch Set 8:
it is hard to review this patch. Most of changes here leave me with a good gut feeling, but it's a leap of faith without proper rationale and possibly some numbers to prove the changes are indeed optimizations.
Moreover, I still have the feeling that this patch squashes together too many vaguely related changes.
Piotr Kliczewski has posted comments on this change.
Change subject: stomp: Optimize stomp encoder\decoder ......................................................................
Patch Set 8:
From my perspective it is not really optimization but bug fixing of stomp frame encoding/decoding. I understand that commit message is confusing will update.
automation@ovirt.org has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 9:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 9: Verified+1
Rebased and commit message updated to make it clear what type of changes this patch contains. Verified with engine 3.5
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 9:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/16762/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/16934/ : FAILURE
automation@ovirt.org has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 10:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 10:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/16792/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/16964/ : FAILURE
automation@ovirt.org has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 11:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 11:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/16870/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17042/ : FAILURE
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 12:
Build Started (1/2) -> http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/16960/
automation@ovirt.org has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 12:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 12: Verified+1
Verified with latest engine (master)
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 12:
Build Started (2/2) -> http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17133/
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 12:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/16960/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17133/ : FAILURE
Yaniv Bronhaim has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 12: Code-Review+1
I have to trust you with that one, hopefully some of the unit tests verify that those changes keep same behavior
Dan Kenigsberg has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 12:
(3 comments)
https://gerrit.ovirt.org/#/c/35255/12/lib/yajsonrpc/stomp.py File lib/yajsonrpc/stomp.py:
Line 77: _heartBeatFrame = _HeartBeatFrame() Line 78: Line 79: Line 80: class Frame(object): Line 81: __slots__ = ("headers", "command", "body") this does not belong to this patch. it's an unrelated performance optimization. Line 82: Line 83: def __init__(self, command="", headers=None, body=None): Line 84: self.command = command Line 85: if headers is None:
Line 117: def copy(self): Line 118: return Frame(self.command, self.headers.copy(), self.body) Line 119: Line 120: Line 121: def decodeValue(s): the encodeValue/decodeValue functions just beg to have a unit test, making sure that they are reversible and that the current re-implementation changed nothing in their behaviour. Line 122: # Make sure to leave this check before decoding as ':' can appear in the Line 123: # value after decoding using \c Line 124: if ":" in s: Line 125: raise ValueError("Contains illigal charachter `:`")
Line 138: def encodeValue(s): Line 139: if isinstance(s, unicode): Line 140: s = s.encode('utf-8') Line 141: elif not isinstance(s, str): Line 142: s = str(s) why do we want this? it makes encodeValue non-reversible. Normally, I'd rather explode if we are asked to encode an integer or some other crazy object. Line 143: Line 144: return _RE_ENCODE_CHARS.sub(lambda m: _EC_ENCODE_MAP[m.group(0)], s) Line 145: Line 146:
Piotr Kliczewski has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 12:
(3 comments)
https://gerrit.ovirt.org/#/c/35255/12/lib/yajsonrpc/stomp.py File lib/yajsonrpc/stomp.py:
Line 77: _heartBeatFrame = _HeartBeatFrame() Line 78: Line 79: Line 80: class Frame(object): Line 81: __slots__ = ("headers", "command", "body")
this does not belong to this patch. it's an unrelated performance optimizat
Will move to separate patch. Line 82: Line 83: def __init__(self, command="", headers=None, body=None): Line 84: self.command = command Line 85: if headers is None:
Line 117: def copy(self): Line 118: return Frame(self.command, self.headers.copy(), self.body) Line 119: Line 120: Line 121: def decodeValue(s):
the encodeValue/decodeValue functions just beg to have a unit test, making
Done Line 122: # Make sure to leave this check before decoding as ':' can appear in the Line 123: # value after decoding using \c Line 124: if ":" in s: Line 125: raise ValueError("Contains illigal charachter `:`")
Line 138: def encodeValue(s): Line 139: if isinstance(s, unicode): Line 140: s = s.encode('utf-8') Line 141: elif not isinstance(s, str): Line 142: s = str(s)
why do we want this? it makes encodeValue non-reversible. Normally, I'd rat
Will improve Line 143: Line 144: return _RE_ENCODE_CHARS.sub(lambda m: _EC_ENCODE_MAP[m.group(0)], s) Line 145: Line 146:
Dan Kenigsberg has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 12: Code-Review-1
automation@ovirt.org has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 13:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 13:
Build Started (1/2) -> http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/17579/
Piotr Kliczewski has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 13: Verified+1
Verified by running with the latest engine. Host installed, NFS domain configured and vm provisioned.
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 13:
Build Started (2/2) -> http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17753/
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 13:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/17579/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17753/ : SUCCESS
Francesco Romani has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 13:
(3 comments)
https://gerrit.ovirt.org/#/c/35255/13//COMMIT_MSG Commit Message:
Line 7: stomp: Stomp encoder\decoder improvements Line 8: Line 9: We need to make sure to escape special characters during encoding and Line 10: decoding. Line 11: During encoding we want to use string join vs using '+' due to its maybe reads better as
"During encoding, use more efficient string join instead of string concatenation"
or something like this.
Please note that strictly speaking, these are still two separate changes squashed together: 1. proper escape in encoding/decoding 2. use string join for speed
I'll not fight to have them split, however, even though I think it will be nicer that way. Line 12: speed. Line 13: Line 14: Change-Id: I341e46463e654ae0a086cd49af18aa336b86cb6e Line 15: Signed-off-by: Saggi Mizrahi smizrahi@redhat.com
https://gerrit.ovirt.org/#/c/35255/13/lib/yajsonrpc/stomp.py File lib/yajsonrpc/stomp.py:
Line 199: Line 200: if len(header) > 0 and header[-1] == '\r': Line 201: header = header[:-1] Line 202: Line 203: headers = self._tmpFrame.headers why this was moved below here? Line 204: if header == "": Line 205: self._contentLength = int(headers.get('content-length', -1)) Line 206: self._state = self._STATE_BODY Line 207: return True
https://gerrit.ovirt.org/#/c/35255/13/tests/encodingTests.py File tests/encodingTests.py:
Line 20: from testlib import ( Line 21: VdsmTestCase as TestCaseBase, Line 22: expandPermutations, Line 23: permutations Line 24: ) A little golang-ish, we don't usually write like this - but I don't really mind. Line 25: from yajsonrpc.stomp import decodeValue, encodeValue Line 26: Line 27: PERMUTATIONS = (('accept-version',), Line 28: ('1.2:',),
Piotr Kliczewski has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 13:
(3 comments)
https://gerrit.ovirt.org/#/c/35255/13//COMMIT_MSG Commit Message:
Line 7: stomp: Stomp encoder\decoder improvements Line 8: Line 9: We need to make sure to escape special characters during encoding and Line 10: decoding. Line 11: During encoding we want to use string join vs using '+' due to its
maybe reads better as
Will update the message Line 12: speed. Line 13: Line 14: Change-Id: I341e46463e654ae0a086cd49af18aa336b86cb6e Line 15: Signed-off-by: Saggi Mizrahi smizrahi@redhat.com
https://gerrit.ovirt.org/#/c/35255/13/lib/yajsonrpc/stomp.py File lib/yajsonrpc/stomp.py:
Line 199: Line 200: if len(header) > 0 and header[-1] == '\r': Line 201: header = header[:-1] Line 202: Line 203: headers = self._tmpFrame.headers
why this was moved below here?
The only reason I see that original author had was that is was not used earlier so the line was moved just to have it before 'headers' is used. Line 204: if header == "": Line 205: self._contentLength = int(headers.get('content-length', -1)) Line 206: self._state = self._STATE_BODY Line 207: return True
https://gerrit.ovirt.org/#/c/35255/13/tests/encodingTests.py File tests/encodingTests.py:
Line 20: from testlib import ( Line 21: VdsmTestCase as TestCaseBase, Line 22: expandPermutations, Line 23: permutations Line 24: )
A little golang-ish, we don't usually write like this - but I don't really
OK Line 25: from yajsonrpc.stomp import decodeValue, encodeValue Line 26: Line 27: PERMUTATIONS = (('accept-version',), Line 28: ('1.2:',),
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 14:
Build Started (1/2) -> http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/17619/
automation@ovirt.org has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 14:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 14: Verified+1
Verified by updating vdsm and seeing that there are no issues during communication.
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 14:
Build Started (2/2) -> http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17793/
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 14:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/17619/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17793/ : SUCCESS
Yaniv Bronhaim has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 14: Code-Review+1
all comments are covered in this ps. pleaes re-visit guys
automation@ovirt.org has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 15:
* Update tracker::IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])
Piotr Kliczewski has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 15: Verified+1
Rebase only, restoring verify flag.
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 15:
Build Started (1/2) -> http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/17782/
Yaniv Bronhaim has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 15: Code-Review+1
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 15:
Build Started (2/2) -> http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/17952/
Francesco Romani has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 15:
(1 comment)
seems fine, unfortunately I noticed use of strings only now. May be nothing worrysome, however.
https://gerrit.ovirt.org/#/c/35255/15/lib/yajsonrpc/stomp.py File lib/yajsonrpc/stomp.py:
Line 41: ":": "\c", Line 42: "\": "\\", Line 43: "\r": "\r", Line 44: "\n": "\n", Line 45: } the fact you're not using traw strings (r"a string") raises a flag. Is that intentional?
https://docs.python.org/2/reference/lexical_analysis.html#string-literals (see second and last period in particular) Line 46: Line 47: Line 48: class Command: Line 49: MESSAGE = "MESSAGE"
Piotr Kliczewski has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 15:
(1 comment)
https://gerrit.ovirt.org/#/c/35255/15/lib/yajsonrpc/stomp.py File lib/yajsonrpc/stomp.py:
Line 41: ":": "\c", Line 42: "\": "\\", Line 43: "\r": "\r", Line 44: "\n": "\n", Line 45: }
the fact you're not using traw strings (r"a string") raises a flag. Is that
It is. I wrote a test to check whether we can decode it and then encode it so I had to update decode map. Line 46: Line 47: Line 48: class Command: Line 49: MESSAGE = "MESSAGE"
Francesco Romani has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 15: Code-Review+1
(1 comment)
https://gerrit.ovirt.org/#/c/35255/15/lib/yajsonrpc/stomp.py File lib/yajsonrpc/stomp.py:
Line 41: ":": "\c", Line 42: "\": "\\", Line 43: "\r": "\r", Line 44: "\n": "\n", Line 45: }
It is. I wrote a test to check whether we can decode it and then encode it
ok Line 46: Line 47: Line 48: class Command: Line 49: MESSAGE = "MESSAGE"
Dan Kenigsberg has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 15: Code-Review+2
Dan Kenigsberg has submitted this change and it was merged.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
stomp: Stomp encoder\decoder improvements
We need to make sure to escape special characters during encoding and decoding. During encoding, use more efficient string join instead of string concatenation due to its speed.
Change-Id: I341e46463e654ae0a086cd49af18aa336b86cb6e Signed-off-by: Saggi Mizrahi smizrahi@redhat.com Signed-off-by: pkliczewski piotr.kliczewski@gmail.com Reviewed-on: https://gerrit.ovirt.org/35255 Reviewed-by: Yaniv Bronhaim ybronhei@redhat.com Reviewed-by: Francesco Romani fromani@redhat.com Reviewed-by: Dan Kenigsberg danken@redhat.com --- M lib/yajsonrpc/stomp.py M tests/Makefile.am A tests/encodingTests.py 3 files changed, 74 insertions(+), 22 deletions(-)
Approvals: Piotr Kliczewski: Verified Yaniv Bronhaim: Looks good to me, but someone else must approve Dan Kenigsberg: Looks good to me, approved Francesco Romani: Looks good to me, but someone else must approve
automation@ovirt.org has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 16:
* Update tracker::IGNORE, no Bug-Url found * Set MODIFIED::IGNORE, no Bug-Url found.
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 16:
Build Started (1/12) -> http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/4867/
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 16:
Build Started (2/12) -> http://jenkins.ovirt.org/job/vdsm_master-libgfapi_create-rpms-el6-x86_64_mer...
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 16:
Build Started (3/12) -> http://jenkins.ovirt.org/job/vdsm_master-libgfapi_create-rpms-el7-x86_64_mer...
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 16:
Build Started (4/12) -> http://jenkins.ovirt.org/job/vdsm_master-libgfapi_create-rpms-fc20-x86_64_me...
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 16:
Build Started (5/12) -> http://jenkins.ovirt.org/job/vdsm_master_create-rpms-el6-x86_64_merged/891/
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 16:
Build Started (6/12) -> http://jenkins.ovirt.org/job/vdsm_master_create-rpms-el7-x86_64_merged/895/
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 16:
Build Started (7/12) -> http://jenkins.ovirt.org/job/vdsm_master_create-rpms-fc20-x86_64_merged/893/
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 16:
Build Started (8/12) -> http://jenkins.ovirt.org/job/vdsm_master-libgfapi_create-rpms-fc21-x86_64_me...
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 16:
Build Started (9/12) -> http://jenkins.ovirt.org/job/vdsm_master_create-rpms-fc22-x86_64_merged/35/
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 16:
Build Started (10/12) -> http://jenkins.ovirt.org/job/vdsm_master-libgfapi_create-rpms-fc22-x86_64_me...
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 16:
Build Started (11/12) -> http://jenkins.ovirt.org/job/sshnaidm_vdsm_master_create-rpms-fc21-ppc64_mer...
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 16:
Build Started (12/12) -> http://jenkins.ovirt.org/job/vdsm_master_create-rpms-fc21-x86_64_merged/858/
oVirt Jenkins CI Server has posted comments on this change.
Change subject: stomp: Stomp encoder\decoder improvements ......................................................................
Patch Set 16:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_create-rpms-el6-x86_64_merged/891/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master-libgfapi_create-rpms-fc21-x86_64_me... : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/4867/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_create-rpms-fc21-x86_64_merged/858/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master-libgfapi_create-rpms-el6-x86_64_mer... : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_create-rpms-el7-x86_64_merged/895/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master-libgfapi_create-rpms-fc20-x86_64_me... : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_create-rpms-fc20-x86_64_merged/893/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master-libgfapi_create-rpms-el7-x86_64_mer... : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master-libgfapi_create-rpms-fc22-x86_64_me... : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_create-rpms-fc22-x86_64_merged/35/ : SUCCESS
http://jenkins.ovirt.org/job/sshnaidm_vdsm_master_create-rpms-fc21-ppc64_mer... : SUCCESS
vdsm-patches@lists.fedorahosted.org