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