Liron Aravot has posted comments on this change. Change subject: core: Register all the OVF disks to Storage ......................................................................
Patch Set 4: (6 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 190: } Line 191: } Line 192: } Line 193: Line 194: protected List<OvfEntityData> getEntitiesFromStorageOvfDisk() { method name no longer correct. Line 195: List<OvfEntityData> ovfEntitiesFromTar = Collections.emptyList(); Line 196: // Get all unregistered disks. Line 197: List<Disk> unregisteredDisks = getBackend().runInternalQuery(VdcQueryType.GetUnregisteredDisks, Line 198: new GetUnregisteredDisksQueryParameters(getParameters().getStorageDomainId(), 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()) { how does transactivity is handled here and how it aligns with the compensation of this command? for example, if the engine crashes after executing part of the needed registrations of disks here before the command ended? Line 252: result = "failed"; Line 253: } Line 254: } catch (Exception e) { Line 255: result = "failed"; Line 250: try { Line 251: if (!runInternalAction(VdcActionType.RegisterDisk, registerDiskParams, null).getSucceeded()) { Line 252: result = "failed"; Line 253: } Line 254: } catch (Exception e) { > I tend to agree, although you might fail here on reflection exception when when there is a sufficent contructor in the class the entire execution should fail, it shouldn't be catched here and obviously that the code shouldn't handle such cases. there's no need for the boolean as the execution of the register disk logs wether it succeeded or failed, you can log it if you want for case of failure, but i don't see any real need for that. 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}", > It can be also for success you'll see that in the log from the logging of the execution of the command. 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(); > no, you will have NullPointerException at line 325 So the problem here is deeper, Date() ctor initiates it to the data in which the object was created. so the condition in line 325 (date.after(foundOvfDiskUpdateDate)) will never be true, no? 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); > Which previous method? registerAllOvfDisks 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
