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 <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
