Tal Nisan has posted comments on this change. Change subject: core: Intrdoucing UploadStream capabillity ......................................................................
Patch Set 6: (8 comments) http://gerrit.ovirt.org/#/c/23460/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UploadStreamCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UploadStreamCommand.java: Line 41: // add message that there's no spm for the pool Line 42: return false; Line 43: } Line 44: Line 45: setStoragePool(null); Why do you force reloading of the storage pool here? Line 46: if (getStoragePool() == null) { Line 47: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_POOL_NOT_EXIST); Line 48: } Line 49: Line 50: if (!validate(new StoragePoolValidator(getStoragePool()).isUp())) { Line 51: return false; Line 52: } Line 53: Line 54: if (!getPoolSpmId().equals(getStoragePool().getspm_vds_id())) { Is there a reason for the SPM to be changed from the first check in line 41? Line 55: // add message that the host isn't the spm Line 56: return false; Line 57: } Line 58: http://gerrit.ovirt.org/#/c/23460/6/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/UploadStreamVDSCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/UploadStreamVDSCommand.java: Line 39: @Override Line 40: protected void executeVdsBrokerCommand() { Line 41: log.info("-- executeVdsBrokerCommand: "); Line 42: log.infoFormat("-- upload parameters:" + "\r\n" Line 43: + " dstSpUUID={0}" + "\r\n" Better use \t\t\t instead of spaces, makes it more readable please you don't have to count spaces Line 44: + " dstSdUUID={1}" + "\r\n" Line 45: + " dstImageGUID={2}" + "\r\n" Line 46: + " dstVolUUID={3}" + "\r\n", Line 47: getParameters().getStoragePoolId().toString(), Line 70: inputStreamRequestEntity = Line 71: new InputStreamRequestEntity(getParameters().getInputStream(), Line 72: getParameters().getStreamLength()); Line 73: } else { Line 74: inputStreamRequestEntity = new InputStreamRequestEntity((getParameters().getInputStream())); iirc the VDSM implementation will not accept a stream without length and will throw HTTP error 411, so why sending it to begin with? Line 75: } Line 76: Line 77: putMethod.setRequestEntity(inputStreamRequestEntity); Line 78: putMethod.setRequestHeader("Content-Type", "application/octet-stream"); Line 74: inputStreamRequestEntity = new InputStreamRequestEntity((getParameters().getInputStream())); Line 75: } Line 76: Line 77: putMethod.setRequestEntity(inputStreamRequestEntity); Line 78: putMethod.setRequestHeader("Content-Type", "application/octet-stream"); Please use HttpHeaders.CONTENT_TYPE from the 'org.apache.http' package, no need to use a String literal here, rather use the standard constants Line 79: if (getParameters().getStreamLength() != null) { Line 80: putMethod.setRequestHeader("Content-Length", getParameters().getStreamLength().toString()); Line 81: } Line 82: Line 76: Line 77: putMethod.setRequestEntity(inputStreamRequestEntity); Line 78: putMethod.setRequestHeader("Content-Type", "application/octet-stream"); Line 79: if (getParameters().getStreamLength() != null) { Line 80: putMethod.setRequestHeader("Content-Length", getParameters().getStreamLength().toString()); Please use HttpHeaders.CONTENT_LENGTH Line 81: } Line 82: Line 83: putMethod.setRequestHeader("connection", "close"); Line 84: putMethod.setRequestHeader("X-Storage-Pool-Id", getParameters().getStoragePoolId().toString()); Line 79: if (getParameters().getStreamLength() != null) { Line 80: putMethod.setRequestHeader("Content-Length", getParameters().getStreamLength().toString()); Line 81: } Line 82: Line 83: putMethod.setRequestHeader("connection", "close"); Please use HttpHeaders.CONNECTION Line 84: putMethod.setRequestHeader("X-Storage-Pool-Id", getParameters().getStoragePoolId().toString()); Line 85: putMethod.setRequestHeader("X-Storage-Domain-Id", getParameters().getStorageDomainId().toString()); Line 86: putMethod.setRequestHeader("X-Image-Id", getParameters().getImageGroupId().toString()); Line 87: putMethod.setRequestHeader("X-Volume-Id", getParameters().getImageId().toString()); Line 97: if (responseCode != HttpStatus.SC_OK) { Line 98: throwVdsErrorException("UploadStreamVDSCommand - upload failed with response code " + responseCode); Line 99: } Line 100: Line 101: processResponseHeaderValue(putMethod, "Content-type", "application/json"); Same here Line 102: Line 103: String response = null; Line 104: try { Line 105: response = putMethod.getResponseBodyAsString(); -- To view, visit http://gerrit.ovirt.org/23460 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7af6e7b580e8844c1f0cd661d7f62ece688e31d3 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Ayal Baron <aba...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Sergey Gotliv <sgot...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
