Nir Soffer has posted comments on this change. Change subject: core: BindingXMLRPC - exporting logic out from do_PUT. ......................................................................
Patch Set 3: (4 comments) http://gerrit.ovirt.org/#/c/26740/3/vdsm/BindingXMLRPC.py File vdsm/BindingXMLRPC.py: Line 150: self._performSocketOp(operation) Line 151: Line 152: def _performSocketOp(self, execFunc): Line 153: try: Line 154: execFunc() > nir, i don't mind to change to code that you'll think that'll be cleaner. b Avoiding duplication is nice but is not enough. The code must be clear and easy to understand. It it nice if there is no duplicate code. About concrete suggestions - just drop this patch and leave the old code as is. Write the new code like he old code in a clear and simple way. Then if there is a need, we can refctor out some simple helper methods or classes. This refactoring is not required to get this feature merged; it just slow down the process. 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): > i would like the name to reflect that the image is created from the request Then it should be _constructIageFromRequestHeaders. But because there is only one way to create an image, there is not need for the longer name. 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 176: return API.Image(imgUUID, spUUID, sdUUID) Line 177: Line 178: @staticmethod Line 179: def _createEventWithCallback(self): Line 180: opFinishedEvent = threading.Event() > I'll move the methods from here to a operation class, where do you want me Currently this module seem to be the right place, but not as a nested class like the request hadler. In a future change we should move the server and request handler out of this module, where infra guys want to maintain it. Line 181: Line 182: def setCallback(): Line 183: opFinishedEvent.set() Line 184: Line 210: return Line 211: Line 212: return value Line 213: Line 214: def handle_response(self, headers, body=None): > I'll change the name of to be without "handle", i prefer to write the body Repeating the self.wfile.write() line *is* good. This help you to understand what the code does. We are not trying to create the minimal number of lines without any duplication, but to create very clear code that will be easy to maintain. 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: 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
