Change in vdsm[master]: Allow GZIP transport on XMLRPC
nsoffer at redhat.com
nsoffer at redhat.com
Fri Feb 28 14:43:25 UTC 2014
Nir Soffer has posted comments on this change.
Change subject: Allow GZIP transport on XMLRPC
......................................................................
Patch Set 1:
(11 comments)
Nice idea, needs cleanup.
http://gerrit.ovirt.org/#/c/25201/1/lib/vdsm/utils.py
File lib/vdsm/utils.py:
Line 162: """data -> gzip encoded data
Line 163:
Line 164: Encode data using the gzip content encoding as described in RFC 1952
Line 165: """
Line 166: f = StringIO()
We should use cStringIO.StringIO which is much faster. It can handle only ASCII (or utf-8) input, but so is GzipFile.
Line 167: gzf = gzip.GzipFile(mode="wb", fileobj=f, compresslevel=1)
Line 168: gzf.write(data)
Line 169: gzf.close()
Line 170: encoded = f.getvalue()
Line 163:
Line 164: Encode data using the gzip content encoding as described in RFC 1952
Line 165: """
Line 166: f = StringIO()
Line 167: gzf = gzip.GzipFile(mode="wb", fileobj=f, compresslevel=1)
We may like different compression level - did you do any testing with common payloads?
Line 168: gzf.write(data)
Line 169: gzf.close()
Line 170: encoded = f.getvalue()
Line 171: f.close()
Line 165: """
Line 166: f = StringIO()
Line 167: gzf = gzip.GzipFile(mode="wb", fileobj=f, compresslevel=1)
Line 168: gzf.write(data)
Line 169: gzf.close()
Should put this try-finally:
gzf = gzip.GzipFile(...)
try:
gzf.write(data)
finally:
gzf.close()
Line 170: encoded = f.getvalue()
Line 171: f.close()
Line 172: return encoded
Line 173:
Line 167: gzf = gzip.GzipFile(mode="wb", fileobj=f, compresslevel=1)
Line 168: gzf.write(data)
Line 169: gzf.close()
Line 170: encoded = f.getvalue()
Line 171: f.close()
Not sure we need this - StirngIO is just a list that looks like a file, close does nothing interesting. See /usr/lib64/python2.7/StringIO.py
So this could be:
return f.getvalue()
Line 172: return encoded
Line 173:
Line 174:
Line 175: class IPXMLRPCRequestHandler(SimpleXMLRPCRequestHandler):
Line 191: # client does not have any clue that the response was finished.
Line 192: #
Line 193: # These changes were taken from Python 2.7 version of this class. If we are
Line 194: # running on Python 2.7, these changes are not needed, hence we override
Line 195: # the methods only on Python 2.6.
Please update the comment for this section, or move the comment near the relevant parts inside the if bellow.
Line 196:
Line 197: if sys.version_info[:2] == (2, 6):
Line 198: def __init__(self, *args, **kwargs):
Line 199: self.encode_threshold = 1400
Line 196:
Line 197: if sys.version_info[:2] == (2, 6):
Line 198: def __init__(self, *args, **kwargs):
Line 199: self.encode_threshold = 1400
Line 200: SimpleXMLRPCRequestHandler.__init__(self, *args, **kwargs)
There is no need to override __init__ to get self.encode_threshold. Instead just:
encode_threshold = 1400
Line 201:
Line 202: # a re to match a gzip Accept-Encoding
Line 203: aepattern = re.compile(r"""
Line 204: \s* ([^\s;]+) \s* #content-coding
Line 226: # Get arguments by reading body of request.
Line 227: # We read this in chunks to avoid straining
Line 228: # socket.read(); around the 10 or 15Mb mark, some platforms
Line 229: # begin to have problems (bug #792570).
Line 230: max_chunk_size = 10 * 1024 * 1024
Not related - please split refactoring to another patch.
Line 231: size_remaining = int(self.headers["content-length"])
Line 232: L = []
Line 233: while size_remaining:
Line 234: chunk_size = min(size_remaining, max_chunk_size)
Line 261: else:
Line 262: # got a valid XML RPC response
Line 263: self.send_response(200)
Line 264: self.send_header("Content-type", "text/xml")
Line 265: pureLen = len(response)
Unused
Line 266: if self.encode_threshold is not None:
Line 267: if pureLen > self.encode_threshold:
Line 268: q = self.accept_encodings().get("gzip", 0)
Line 269: if q:
Line 265: pureLen = len(response)
Line 266: if self.encode_threshold is not None:
Line 267: if pureLen > self.encode_threshold:
Line 268: q = self.accept_encodings().get("gzip", 0)
Line 269: if q:
This should be extracted out:
def should_encode(self, response):
return (self.encode_threshold is not None and
self.encode_threshold < len(response) and
self.accept_encodings().get("gzip", 0))
Line 270: response = gzip_encode(response)
Line 271: self.send_header("Content-encoding", "gzip")
Line 272: self.send_header("Content-length", str(len(response)))
Line 273: self.end_headers()
Line 274: self.wfile.write(response)
Line 275: import datetime
Line 276: import re
Line 277: with open('/tmp/rpc-stats.log', 'a') as statslog:
Line 278: statslog.write("%s - %s - %d - %d\n" % (datetime.datetime.utcnow().isoformat(), re.search(r'(?<=<methodName>)(.+)(?=<\/methodName)', data).group(0), len(response), pureLen))
Remove this debugging code
Line 279:
Line 280: def report_404(self):
Line 281: self.send_response(404)
Line 282: response = 'No such page'
http://gerrit.ovirt.org/#/c/25201/1/lib/vdsm/vdscli.py.in
File lib/vdsm/vdscli.py.in:
Line 49: gzip.GzipFile.__init__(self, mode="rb", fileobj=self.stringio)
Line 50:
Line 51: def close(self):
Line 52: gzip.GzipFile.close(self)
Line 53: self.stringio.close()
I don't think we need this class.
We need to read the entire response using the given content-length, then decode it like we do in the xmlrpc bindings.
Line 54:
Line 55: def wrap_request(transport):
Line 56: self = transport
Line 57:
--
To view, visit http://gerrit.ovirt.org/25201
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebdaa04b17e2c1df1c1852ed536c5d6d8ec8d88b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer 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