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 <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: Yoav Kleinberger <[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
