Nir Soffer has posted comments on this change. Change subject: core: BindingXMLRPC - exporting logic out from do_PUT. ......................................................................
Patch Set 8: Code-Review-1 (14 comments) http://gerrit.ovirt.org/#/c/26740/8/vdsm/BindingXMLRPC.py File vdsm/BindingXMLRPC.py: Line 34: from vdsm.exception import VdsmException Line 35: Line 36: try: Line 37: from gluster.api import getGlusterMethods Line 38: Why these unrelated changes are here? Line 39: _glusterEnabled = True Line 40: except ImportError: Line 41: _glusterEnabled = False Line 42: Line 107: else: Line 108: basehandler = SimpleXMLRPCServer.SimpleXMLRPCRequestHandler Line 109: Line 110: class RequestHandler(basehandler): Line 111: # Timeout for the request socket Why this whitespace change is there? Line 112: timeout = 60 Line 113: log = logging.getLogger("BindingXMLRPC.RequestHandler") Line 114: Line 115: HEADER_POOL = 'Storage-Pool-Id' Line 123: HEADER_CONTENT_TYPE = 'content-type' Line 124: Line 125: class RequestException(): Line 126: def __init__(self, httpError, errorMessage): Line 127: self.error = httpError This is not an error but http status code. Line 128: self.message = errorMessage Line 129: Line 130: def setup(self): Line 131: threadLocal.client = self.client_address[0] Line 132: threadLocal.server = self.request.getsockname()[0] Line 133: return basehandler.setup(self) Line 134: Line 135: def do_PUT(self): Line 136: This whitespace change is both unwanted and unrelated to the patch; Line 137: try: Line 138: contentLength = self._getIntHeader( Line 139: self.HEADER_CONTENT_LENGTH, Line 140: httplib.LENGTH_REQUIRED) Line 136: Line 137: try: Line 138: contentLength = self._getIntHeader( Line 139: self.HEADER_CONTENT_LENGTH, Line 140: httplib.LENGTH_REQUIRED) This method does not make sense, receiving the http status code it should raise in case of an error. This helper can fail in two cases: - No such header - Header value is not an int Since there is only one place where we want to handle each error differently, this should be something like this: try: contentLength = self._get_int_header(self.HEADER_CONTENT_LENGTH) except self.NoSuchHeader: raise RequestException(httplib.LENGTH_REQUIRED) except self.InvalidHeader: raise RequestException(httplib.BAD_REQUEST) If you want to simplify this code, you can use the same bad request error for both cases, which is good enough - you do this in the do_GET. In this case, _get_int_header can simply raise RequestException(httplib.BAD_REQUEST). I think the simpler code is much better in this case - nobody cares about getting the httplib.LENGTH_REQUIRED when you try to use our api in a the wrong way. Line 141: Line 142: img = self._createImage() Line 143: Line 144: methodArgs = {'fileObj': self.rfile, Line 146: # Optional header Line 147: volUUID = self.headers.getheader(self.HEADER_VOLUME) Line 148: Line 149: uploadFinishedEvent, operationEndCallback = \ Line 150: self._createEventWithCallback() Lets create the variables in the same order we call them: methodArgs operationEndCallback volUUID Line 151: response = img.downloadFromStream(methodArgs, Line 152: operationEndCallback, Line 153: volUUID) Line 154: Line 165: Line 166: except self.RequestException as e: Line 167: self.send_error(e.error, Line 168: e.message) Line 169: return Line break and return not needed here. Line 170: Line 171: except Exception: Line 172: self.send_error(httplib.INTERNAL_SERVER_ERROR, Line 173: "error during execution", Line 186: % (spUUID, sdUUID, imgUUID)) Line 187: Line 188: return API.Image(imgUUID, spUUID, sdUUID) Line 189: Line 190: @staticmethod Private static method does not make sense in Python. Line 191: def _createEventWithCallback(): Line 192: operationFinishedEvent = threading.Event() Line 193: Line 194: def setCallback(): Line 195: operationFinishedEvent.set() Line 196: Line 197: return operationFinishedEvent, setCallback Line 198: Line 199: @staticmethod Private static method does not make sense in Python. Line 200: def _waitForEvent(event): Line 201: while not event.is_set(): Line 202: event.wait() Line 203: Line 198: Line 199: @staticmethod Line 200: def _waitForEvent(event): Line 201: while not event.is_set(): Line 202: event.wait() Both method above have nothing to do with this class - it would be better to put them somewhere else. As I suggested before - having a separate class for these makes more sense: class Operation(object): def __init__(self): self._event = threading.Event() def wait(self): while not self._event.is_set(): self._event.wait() def finished(self): self._event.set() This is simpler, shorter and make more sense than _createEventWithCallback() and _waitForEvent. And the code using it is much more clear: upload = Operation() img.uploadToStream(upload.finished, ...) upload.wait() Line 203: Line 204: def _getIntHeader(self, headerName, missingError): Line 205: value = self.headers.getheader( Line 206: headerName) Line 225: Line 226: def send_error(self, error, message, exc_info=False): Line 227: try: Line 228: self.log.error(message, exc_info=exc_info) Line 229: self.send_response(error) Why did you remove self.end_headers() ?! Line 230: except Exception: Line 231: self.log.error("failed to return response", Line 232: exc_info=True) Line 233: raise Line 229: self.send_response(error) Line 230: except Exception: Line 231: self.log.error("failed to return response", Line 232: exc_info=True) Line 233: raise How is this related to this change? Regarding the change, why do you think that we like 2 tracebacks - one logged by this logger, and the other logged by the caller of do_PUT? Line 234: Line 235: def parse_request(self): Line 236: r = basehandler.parse_request(self) Line 237: threadLocal.flowID = self.headers.get(HTTP_HEADER_FLOWID) Line 1080: Line 1081: # Logging current call Line 1082: logStr = 'client [%s]::call %s with %s %s' % \ Line 1083: (getattr(f.im_self.cif.threadLocal, 'client', ''), Line 1084: f.__name__, displayArgs, kwargs) How is this whitespace change related? Line 1085: Line 1086: # if flowID exists Line 1087: if getattr(f.im_self.cif.threadLocal, 'flowID', None) is not None: Line 1088: logStr += " flowID [%s]" % f.im_self.cif.threadLocal.flowID Line 1108: return e.response() Line 1109: except: Line 1110: f.im_self.cif.log.error("unexpected error", exc_info=True) Line 1111: return errCode['unexpected'] Line 1112: How is this whitespace change related? Line 1113: wrapper.__name__ = f.__name__ Line 1114: wrapper.__doc__ = f.__doc__ -- 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: 8 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
