Change in vdsm[master]: core: BindingXMLRPC - exporting logic out from do_PUT.

nsoffer at redhat.com nsoffer at redhat.com
Tue May 13 13:52:35 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: core: BindingXMLRPC - exporting logic out from do_PUT.
......................................................................


Patch Set 3: Code-Review-1

(9 comments)

http://gerrit.ovirt.org/#/c/26740/3/vdsm/BindingXMLRPC.py
File vdsm/BindingXMLRPC.py:

Line 126:                 return basehandler.setup(self)
Line 127: 
Line 128:             def do_PUT(self):
Line 129:                 contentLength = self._getIntHeader(self.HEADER_CONTENT_LENGTH,
Line 130:                                                    httplib.LENGTH_REQUIRED)
This should return in this level ending the request.
Line 131:                 image = self._constructImageFromRequestInfo()
Line 132:                 methodArgs = {'fileObj': self.rfile,
Line 133:                               'length': contentLength}
Line 134:                 # Optional header


Line 150:                 self._performSocketOp(operation)
Line 151: 
Line 152:             def _performSocketOp(self, execFunc):
Line 153:                 try:
Line 154:                     execFunc()
This abstractions are impossible to understand.

The previous code was very clear - the only thing needed is some simple helper methods to make the logic more clear and hide the details.

Something like:

    try:
        create image
        get optional parameter for task
        start copy to image task
        wait until task finished
        send response
    except Exception:
        send error
Line 155: 
Line 156:                 except socket.timeout:
Line 157:                     self.send_error(httplib.REQUEST_TIMEOUT,
Line 158:                                     "request timeout")


Line 160:                 except Exception:
Line 161:                     self.send_error(httplib.INTERNAL_SERVER_ERROR,
Line 162:                                     "error during execution", exc_info=True)
Line 163: 
Line 164:             def _constructImageFromRequestInfo(self):
What is RequestInfo? There is only one way to create image, so createImageFromXXX does not add any value to the reader. This should be _createImage.
Line 165:                 # Required headers
Line 166:                 spUUID = self.headers.getheader(self.HEADER_POOL)
Line 167:                 sdUUID = self.headers.getheader(self.HEADER_DOMAIN)
Line 168:                 imgUUID = self.headers.getheader(self.HEADER_IMAGE)


Line 169:                 if not all((spUUID, sdUUID, imgUUID)):
Line 170:                     self.send_error(httplib.BAD_REQUEST,
Line 171:                                     "missing or empty required header(s):"
Line 172:                                     " spUUID=%s sdUUID=%s imgUUID=%s"
Line 173:                                     % (spUUID, sdUUID, imgUUID))
Sending error in a helper method make it very hard to follow the code or to use the helper correctly. 

If the helper fail, it should throw. The caller of the helper should send error.
Line 174:                     return
Line 175: 
Line 176:                 return API.Image(imgUUID, spUUID, sdUUID)
Line 177: 


Line 176:                 return API.Image(imgUUID, spUUID, sdUUID)
Line 177: 
Line 178:             @staticmethod
Line 179:             def _createEventWithCallback(self):
Line 180:                 opFinishedEvent = threading.Event()
This helper just make it harder to use the event. It does eliminate the need to define the callback in the caller, but you pay for this by having to use api that nobody knows, instead of using the standard Event api.

Instead of this, you can simply do:

    uploadFinished = Event()

    image.copyToImage(callback=uploadFinished.set)

    while not uploadFinished.is_set():
        uploadFinished.wait()

The reason we have to write the loop above is bug in our pthreading module - regular Python Event should not return before the event is set. We can remove this loop when a new version of pthreading is available with this fix.

If you want to cleanup the current code, you can create a wrapper class like this:

    class Operation(object):

        def __init__(self):
            self.event = Event()

        def finished(self):
            self.event.set()

        def waitUntilFinished(self):
            while not self.event.is_set():
                self.event.wait()

Note that this class does not support timeout - for this the fix will be in ptreading.


Then we can use it like this:

    operation = Operation()
    image.copyToImage(callback=opetaion.finished)
    operation.wait()
Line 181: 
Line 182:                 def setCallback():
Line 183:                     opFinishedEvent.set()
Line 184: 


Line 186: 
Line 187:             @staticmethod
Line 188:             def _waitForEvent(event):
Line 189:                 while not event.is_set():
Line 190:                     event.wait()
See the comment above.
Line 191: 
Line 192:             @staticmethod
Line 193:             def _convertToJson(data):
Line 194:                 return json.dumps(data)


Line 190:                     event.wait()
Line 191: 
Line 192:             @staticmethod
Line 193:             def _convertToJson(data):
Line 194:                 return json.dumps(data)
Why do we need this method?
Line 195: 
Line 196:             def _getIntHeader(self, headerName, missingError):
Line 197:                 value = self.headers.getheader(
Line 198:                     headerName)


Line 208:                                     "invalid header value %r" %
Line 209:                                     value)
Line 210:                     return
Line 211: 
Line 212:                 return value
Should raise, not send errors.
Line 213: 
Line 214:             def handle_response(self, headers, body=None):
Line 215:                 self.send_response(httplib.OK)
Line 216: 


Line 210:                     return
Line 211: 
Line 212:                 return value
Line 213: 
Line 214:             def handle_response(self, headers, body=None):
This must be private, and must not use handle. The world handle is used for event handlers, and it just confusing.

This abstraction is also wrong for http response. A response has headers and body - if we need a helper, it is for the header of the response:

    def _send_response_header(status, headers):
        self.send_response(status)
        for k, v in headers.iteritems():
            self.send_header(k, v)
        self.end_headers()

Using this helper, code will do:

    self._send_response_header(headers)
    
    # If there is body
    self.wfile.write(body)

If you think it will be more clear, you can add a helper for the body:

    def _send_response_body(self, data):
         self.wfile.write(body)
Line 215:                 self.send_response(httplib.OK)
Line 216: 
Line 217:                 for k, v in headers.iteritems():
Line 218:                     self.send_header(k, v)


-- 
To view, visit http://gerrit.ovirt.org/26740
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I64bb1b0a4cb85ce822929f1907847dd63eb69fc2
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Liron Ar <laravot at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan at redhat.com>
Gerrit-Reviewer: Yoav Kleinberger <ykleinbe 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