Maor Lipchuk has posted comments on this change. Change subject: core: Initialize entities in DB from OVF disk on attach ......................................................................
Patch Set 6: (11 comments) http://gerrit.ovirt.org/#/c/29041/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AttachStorageDomainToPoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AttachStorageDomainToPoolCommand.java: Line 172: } Line 173: } Line 174: Line 175: protected List<OvfEntityData> getEntitiesFromStorageOvfDisk() { Line 176: List<OvfEntityData> ovfEntitiesFromTar = new ArrayList<>(); > initialize it to Collections.emptyList(). done Line 177: Line 178: // Get all unregistered disks. Line 179: List<Disk> unregisteredDisks = getBackend().runInternalQuery(VdcQueryType.GetUnregisteredDisks, Line 180: new GetUnregisteredDisksQueryParameters(getParameters().getStorageDomainId(), Line 208: * - A list of disks Line 209: * @return A Pair which contains the best OVF disk to retrieve data from and its size. Line 210: */ Line 211: private Pair<DiskImage, Long> getLatestOVFDisk(List<Disk> disks) { Line 212: Date lastUpdate = new Date(); > rename to foundOvfDiskUpdateDate done Line 213: boolean lastIsUpdated = false; Line 214: Long size = 0L; Line 215: Disk diskToGetOVF = null; Line 216: for (Disk disk : disks) { Line 209: * @return A Pair which contains the best OVF disk to retrieve data from and its size. Line 210: */ Line 211: private Pair<DiskImage, Long> getLatestOVFDisk(List<Disk> disks) { Line 212: Date lastUpdate = new Date(); Line 213: boolean lastIsUpdated = false; > rename to isFoundOvfDiskUpdated done Line 214: Long size = 0L; Line 215: Disk diskToGetOVF = null; Line 216: for (Disk disk : disks) { Line 217: boolean isDiskLastUpdated = false; Line 211: private Pair<DiskImage, Long> getLatestOVFDisk(List<Disk> disks) { Line 212: Date lastUpdate = new Date(); Line 213: boolean lastIsUpdated = false; Line 214: Long size = 0L; Line 215: Disk diskToGetOVF = null; > rename to ovfDisk done Line 216: for (Disk disk : disks) { Line 217: boolean isDiskLastUpdated = false; Line 218: // Check which disks are of OVF_STORE Line 219: String diskDecription = ((DiskImage) disk).getDescription(); Line 213: boolean lastIsUpdated = false; Line 214: Long size = 0L; Line 215: Disk diskToGetOVF = null; Line 216: for (Disk disk : disks) { Line 217: boolean isDiskLastUpdated = false; > please rename to isBetterOvfDiskFound. done Line 218: // Check which disks are of OVF_STORE Line 219: String diskDecription = ((DiskImage) disk).getDescription(); Line 220: if (diskDecription.contains(OvfInfoFileConstants.OvfStoreDescriptionLabel)) { Line 221: Map<String, Object> diskDescriptionMap = buildJson(diskDecription); Line 217: boolean isDiskLastUpdated = false; Line 218: // Check which disks are of OVF_STORE Line 219: String diskDecription = ((DiskImage) disk).getDescription(); Line 220: if (diskDecription.contains(OvfInfoFileConstants.OvfStoreDescriptionLabel)) { Line 221: Map<String, Object> diskDescriptionMap = buildJson(diskDecription); > if it fails you should continue, catch the exception and add a log in warn done Line 222: if (!isDomainExistsInDiskDescription(diskDescriptionMap, getParameters().getStorageDomainId())) { Line 223: break; Line 224: } Line 225: Line 218: // Check which disks are of OVF_STORE Line 219: String diskDecription = ((DiskImage) disk).getDescription(); Line 220: if (diskDecription.contains(OvfInfoFileConstants.OvfStoreDescriptionLabel)) { Line 221: Map<String, Object> diskDescriptionMap = buildJson(diskDecription); Line 222: if (!isDomainExistsInDiskDescription(diskDescriptionMap, getParameters().getStorageDomainId())) { > please add a comment to clarify that the purpose of this check is to verify done Line 223: break; Line 224: } Line 225: Line 226: boolean isUpdated = Boolean.valueOf(diskDescriptionMap.get(OvfInfoFileConstants.IsUpdated).toString()); Line 219: String diskDecription = ((DiskImage) disk).getDescription(); Line 220: if (diskDecription.contains(OvfInfoFileConstants.OvfStoreDescriptionLabel)) { Line 221: Map<String, Object> diskDescriptionMap = buildJson(diskDecription); Line 222: if (!isDomainExistsInDiskDescription(diskDescriptionMap, getParameters().getStorageDomainId())) { Line 223: break; > why break? continue. done Line 224: } Line 225: Line 226: boolean isUpdated = Boolean.valueOf(diskDescriptionMap.get(OvfInfoFileConstants.IsUpdated).toString()); Line 227: Date date = getDateFromDiskDescription(diskDescriptionMap); Line 225: Line 226: boolean isUpdated = Boolean.valueOf(diskDescriptionMap.get(OvfInfoFileConstants.IsUpdated).toString()); Line 227: Date date = getDateFromDiskDescription(diskDescriptionMap); Line 228: // If last is updated is false, and the current disk was updated, then the current disk is what we are Line 229: // about to update from. > all the checks between 229-238 can be replaced with - I prefer it that way, the final solution will be to make it with list of disks and a private class to encapsulate all the fields of JSON and dsk Line 230: if (!lastIsUpdated && isUpdated) { Line 231: isDiskLastUpdated = true; Line 232: // If also the current disk was not updated, then check which disk has the latest update date. Line 233: } else if (!isUpdated && date.after(lastUpdate)) { Line 253: return new SimpleDateFormat(OvfParser.formatStrFromDiskDescription).parse(map.get(OvfInfoFileConstants.LastUpdated) Line 254: .toString()); Line 255: } catch (java.text.ParseException e) { Line 256: log.errorFormat("LastUpdate Date could not be parsed from disk desscription"); Line 257: e.printStackTrace(); > log the exception in debug. why? this is an error Line 258: return null; Line 259: } Line 260: } Line 261: Line 262: private boolean isDomainExistsInDiskDescription(Map<String, Object> map, Guid storageDomainId) { Line 263: return map.get(OvfInfoFileConstants.Domains).toString().contains(storageDomainId.toString()); Line 264: } Line 265: Line 266: private Map<String, Object> buildJson(String json) { > use the helper for that, you have that exact method there moved this to be used at getLatestOVFDisk Line 267: try { Line 268: return JsonHelper.jsonToMap(json); Line 269: } catch (IOException e) { Line 270: throw new RuntimeException("Exception while generating json containing ovf store info", e); -- To view, visit http://gerrit.ovirt.org/29041 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ica68974916821ac1c9fbe6f43400170d19d716c9 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Tal Nisan <[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
