Change in vdsm[master]: core: introducing uploadImageToStream

nsoffer at redhat.com nsoffer at redhat.com
Mon May 26 20:24:19 UTC 2014


Nir Soffer has posted comments on this change.

Change subject: core: introducing uploadImageToStream
......................................................................


Patch Set 8:

(6 comments)

http://gerrit.ovirt.org/#/c/26741/8/vdsm/BindingXMLRPC.py
File vdsm/BindingXMLRPC.py:

Line 118:             HEADER_VOLUME = 'Volume-Id'
Line 119:             HEADER_TASK_ID = 'Task-Id'
Line 120:             HEADER_CODE = 'Status-Code'
Line 121:             HEADER_MESSAGE = 'Status-Message'
Line 122:             HEADER_SIZE = 'Size'
Size is not specific enough - consider "Image-Size"
Line 123:             HEADER_CONTENT_LENGTH = 'content-length'
Line 124:             HEADER_CONTENT_TYPE = 'content-type'
Line 125: 
Line 126:             class RequestException():


Line 135: 
Line 136:             def do_GET(self):
Line 137: 
Line 138:                 try:
Line 139:                     length = self._getIntHeader(self.HEADER_SIZE,
Why do you call this length, while the header is called Size? We should use the same term, end to end.
Line 140:                                                 httplib.BAD_REQUEST)
Line 141:                     img = self._constructImage()
Line 142:                     startEvent = threading.Event()
Line 143:                     methodArgs = {'fileObj': self.wfile,


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 arguments in the same order we use them in the call:

    methdArgs
    operationEndCallback
    startEvent
    volUUID
Line 151:                     response = img.uploadToStream(methodArgs,
Line 152:                                                   operationEndCallback,
Line 153:                                                   startEvent, volUUID)
Line 154: 


Line 153:                                                   startEvent, volUUID)
Line 154: 
Line 155:                     success = response['status']['code'] == 0
Line 156: 
Line 157:                     if success:
This variable is not helping to understand what this method does. It help to avoid "duplication" of 2 lines of code, but we don't have to pay this price.

It should be something like this:

    if response['status']['code'] == 0:
        send response with ok
        send content type header
        send content length header
        send task id header
        send response headers
        end headers
        signal task to start
        wait for task to finish
    else:
        send response with error
        send response headers
        end headers
Line 158:                         self.send_response(httplib.OK)
Line 159:                         self.send_header(self.HEADER_CONTENT_TYPE,
Line 160:                                          'application/octet-stream')
Line 161:                         self.send_header(self.HEADER_CONTENT_LENGTH, length)


Line 171:                         self._waitForEvent(uploadFinishedEvent)
Line 172: 
Line 173:                 except self.RequestException as e:
Line 174:                     self.send_error(e.error,
Line 175:                                     e.message)
Is this always expected or we like to use exc_info=True?

Line break unneeded.
Line 176:                     return
Line 177: 
Line 178:                 except Exception:
Line 179:                     self.send_error(httplib.INTERNAL_SERVER_ERROR,


Line 172: 
Line 173:                 except self.RequestException as e:
Line 174:                     self.send_error(e.error,
Line 175:                                     e.message)
Line 176:                     return
Return unneeded.
Line 177: 
Line 178:                 except Exception:
Line 179:                     self.send_error(httplib.INTERNAL_SERVER_ERROR,
Line 180:                                     "error during execution",


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

Gerrit-MessageType: comment
Gerrit-Change-Id: If2df4d3a16f39bf80281d7669ed31fd8369bada5
Gerrit-PatchSet: 8
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: Liron Ar <laravot 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