Maor Lipchuk has posted comments on this change. Change subject: core: Register all the OVF disks to Storage ......................................................................
Patch Set 4: (5 comments) http://gerrit.ovirt.org/#/c/32743/4/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 241: } Line 242: Line 243: private void registerAllOvfDisks(List<DiskImage> ovfStoreDiskImages) { Line 244: for (DiskImage ovfStoreDiskImage : ovfStoreDiskImages) { Line 245: ovfStoreDiskImage.setDiskAlias("OVF_STORE"); > there's a constant for that in OvfInfoFileConstants done Line 246: ovfStoreDiskImage.setDiskDescription("OVF_STORE"); Line 247: ovfStoreDiskImage.setShareable(true); Line 248: RegisterDiskParameters registerDiskParams = new RegisterDiskParameters(ovfStoreDiskImage, getParameters().getStorageDomainId()); Line 249: String result = "succeeded"; Line 250: try { Line 251: if (!runInternalAction(VdcActionType.RegisterDisk, registerDiskParams, null).getSucceeded()) { Line 252: result = "failed"; Line 253: } Line 254: } catch (Exception e) { > running internal action can't throw exception, so please eliminate this cat I tend to agree, although you might fail here on reflection exception when there is no sufficient constructor in the class... I prefer to keep the String so I will not have to write the same log twice. Line 255: result = "failed"; Line 256: log.error(e); Line 257: } Line 258: log.infoFormat("Register OVF_STORE disk id {0} for storage domain {1} has {2}", Line 254: } catch (Exception e) { Line 255: result = "failed"; Line 256: log.error(e); Line 257: } Line 258: log.infoFormat("Register OVF_STORE disk id {0} for storage domain {1} has {2}", > there's no need to that in the log, we'll see the log of RegisterDiskComman It can be also for success Line 259: ovfStoreDiskImage.getId(), Line 260: getParameters().getStorageDomainId(), Line 261: result); Line 262: } Line 298: * - A list of OVF_STORE disks Line 299: * @return A Pair which contains the best OVF disk to retrieve data from and its size. Line 300: */ Line 301: private Pair<DiskImage, Long> getLatestOVFDisk(List<DiskImage> ovfStoreDiskImages) { Line 302: Date foundOvfDiskUpdateDate = new Date(); > can be initiated as null no, you will have NullPointerException at line 325 Line 303: boolean isFoundOvfDiskUpdated = false; Line 304: Long size = 0L; Line 305: Disk ovfDisk = null; Line 306: for (DiskImage ovfStoreDisk : ovfStoreDiskImages) { Line 309: // Check which disks are of OVF_STORE Line 310: String diskDecription = ovfStoreDisk.getDescription(); Line 311: Map<String, Object> diskDescriptionMap; Line 312: try { Line 313: diskDescriptionMap = JsonHelper.jsonToMap(diskDecription); > no need to make this conversion twice, the previous method can return a map Which previous method? Line 314: } catch (IOException e) { Line 315: log.warnFormat("Exception while generating json containing ovf store info. Exception: {0}", e); Line 316: continue; Line 317: } -- To view, visit http://gerrit.ovirt.org/32743 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9fc6666a64efa16968935cfceb10e19b8a4e2eef Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
