Maor Lipchuk 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. or better initialize this in the constructor 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 it with all the fields that are needed for the genereal info. I would consider of initializing all those fields in one place. You can initialize preOvfUpdateInfo, and postOvfUpdateInfo in the constructor (instead use it statically) with all those fields 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) 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). 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 instead the four first attributes? 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: 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
