Liron Ar has posted comments on this change. Change subject: core: BindingXMLRPC - exporting logic out from do_PUT. ......................................................................
Patch Set 3: (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. Done Line 131: image = self._constructImageFromRequestInfo() Line 132: methodArgs = {'fileObj': self.rfile, Line 133: 'length': contentLength} Line 134: # Optional header Line 133: 'length': contentLength} Line 134: # Optional header Line 135: volUUID = self.headers.getheader(self.HEADER_VOLUME) Line 136: Line 137: def operation(): > this code is totally cryptic. way too abstract. you can do that refactor in a further patch and it'll be reviewed, this patch is about exporting common logic that already exist to a new method while keeping the changes minimal. Line 138: opEndEvent, opEndCallback = self._createEventWithCallback() Line 139: response = image.downloadFromStream(methodArgs, Line 140: opEndCallback, volUUID) Line 141: json_response = self._convertToJson(response) 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. nir, i don't mind to change to code that you'll think that'll be cleaner. but please make a concrete suggestion. right now that changes are here to avoid duplicacy between two almost identical scenarios and imo it's clear enoguh (we can always add comments as you wish) - if you have suggestions to improvements while avoiding the exact same code in two functions and the need to maintain it in both it'll be great. 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 createImageF i would like the name to reflect that the image is created from the request headers. 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 this is a private helper used only here, imo it's clearer than the suggested change. 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 nee I'll move the methods from here to a operation class, where do you want me to put it? Line 181: Line 182: def setCallback(): Line 183: opFinishedEvent.set() Line 184: Line 208: "invalid header value %r" % Line 209: value) Line 210: return Line 211: Line 212: return value > Should raise, not send errors. commented before. 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 I'll change the name of to be without "handle", i prefer to write the body here as well instead of repeating the line '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) Line 211: Line 212: return value Line 213: Line 214: def handle_response(self, headers, body=None): Line 215: self.send_response(httplib.OK) > handle_response is a bad name. This is not a handler, it is a helper. i'll change the name Line 216: Line 217: for k, v in headers.iteritems(): Line 218: self.send_header(k, v) Line 219: -- 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: Liron Ar <[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
