Liron Ar has posted comments on this change. Change subject: core: set volume description to reflect ovf store status ......................................................................
Patch Set 4: (5 comments) http://gerrit.ovirt.org/#/c/26932/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ProcessOvfUpdateForStorageDomainCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ProcessOvfUpdateForStorageDomainCommand.java: Line 61: postUpdate.put(OvfInfoFileConstants.DiskDescription, OvfInfoFileConstants.OvfStoreDescriptionLabel); Line 62: Line 63: preOvfUpdateInfo = Collections.unmodifiableMap(preUpdate); Line 64: postOvfUpdateInfo = Collections.unmodifiableMap(postUpdate); Line 65: } > please extract this to a static method. related changes in the updated patchset, please take a look. Line 66: Line 67: public ProcessOvfUpdateForStorageDomainCommand(T parameters) { Line 68: super(parameters); Line 69: setStorageDomainId(parameters.getStorageDomainId()); Line 123: Line 124: private String buildOvfGeneralInfoJson(Date updateDate, Map<String, ?> additionalData) { Line 125: Map<String, Object> map = new HashMap<>(); Line 126: map.put(OvfInfoFileConstants.LastUpdated, updateDate.toString()); Line 127: map.put(OvfInfoFileConstants.Domains, Arrays.asList(getParameters().getStorageDomainId())); > I don't think that three is need to create a new map unless you initialize for preOvfUpdateInfo i can't do the initializion in the ctor as each disk as different update time (it's taken from the db), so the map is different for each ovf disk. we can initialize the postOvfUpdateInfo once and re use it..though it's doesn't really matter - related changes in the updated patchset, please take a look. Line 128: map.putAll(additionalData); Line 129: try { Line 130: return JsonHelper.mapToJson(map); Line 131: } catch (IOException e) { Line 214: Guid diskId, Line 215: Guid volumeId, Line 216: StorageDomainOvfInfo storageDomainOvfInfo, Line 217: Map<String, ?> additionalData) { Line 218: try { > the try block should be only at the runVdsCommand (without the parameters) Done Line 219: SetVolumeDescriptionVDSCommandParameters vdsCommandParameters = Line 220: new SetVolumeDescriptionVDSCommandParameters(storagePoolId, storageDomainId, Line 221: diskId, volumeId, buildOvfGeneralInfoJson(storageDomainOvfInfo.getLastUpdated(), Line 222: additionalData)); Line 221: diskId, volumeId, buildOvfGeneralInfoJson(storageDomainOvfInfo.getLastUpdated(), Line 222: additionalData)); Line 223: runVdsCommand(VDSCommandType.SetVolumeDescription, vdsCommandParameters); Line 224: } catch (VdcBLLException e) { Line 225: log.infoFormat("failed to set description for disk {0}, proceeding with execution", diskId); > please use warnFormat, and indicate also the disk alias (if you can). the disk alias is always the same currently (OVF_STORE), so i prefer to keep the id to distinguish between it. Line 226: } Line 227: } Line 228: Line 229: private boolean performOvfUpdateForDomain(byte[] ovfData, Line 237: Guid volumeId = ovfDisk.getImageId(); Line 238: Line 239: storageDomainOvfInfo.setStoredOvfIds(null); Line 240: setOvfVolumeDescription(storagePoolId, storageDomainId, Line 241: diskId, volumeId, storageDomainOvfInfo, preOvfUpdateInfo); > suggestion: Would not it be more simpler to pass only the ovfDisk here inst we already need to use it for executing the upload command in line 248, so i preferred to just export it once rather than do it again in setOvfVolumeDescription Line 242: Line 243: getStorageDomainOvfInfoDao().update(storageDomainOvfInfo); Line 244: Line 245: ByteArrayInputStream byteArrayInputStream = -- To view, visit http://gerrit.ovirt.org/26932 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I09fc7ab550cd86e7843656ba92a5322165703a2d Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liron Ar <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
