Maor Lipchuk has posted comments on this change. Change subject: core: Register all the OVF disks to Storage ......................................................................
Patch Set 6: (3 comments) http://gerrit.ovirt.org/#/c/32743/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 52: @NonTransactiveCommandAttribute(forceCompensation = true) Line 53: public class AttachStorageDomainToPoolCommand<T extends AttachStorageDomainToPoolParameters> extends Line 54: StorageDomainCommandBase<T> { Line 55: private StoragePoolIsoMap map; Line 56: private List<DiskImage> ovfDisks = null; > no need for the = null; Not that crucial, but removed Line 57: Line 58: public AttachStorageDomainToPoolCommand(T parameters) { Line 59: this(parameters, null); Line 60: } Line 248: ovfStoreDiskImage.setShareable(true); Line 249: RegisterDiskParameters registerDiskParams = Line 250: new RegisterDiskParameters(ovfStoreDiskImage, getParameters().getStorageDomainId()); Line 251: String result = "succeeded"; Line 252: if (!runInternalAction(VdcActionType.RegisterDisk, registerDiskParams, null).getSucceeded()) { > 1. you should add a comment to indicate that the same transaction is used b 1. Why? is someone change the transaction, then he should check before, what he is doing?! 2. why? It will be "riding" on the existing transaction, if it will fail all the disks will be removed. Line 253: result = "failed"; Line 254: } Line 255: log.infoFormat("Register new floating OVF_STORE disk with disk id {0} for storage domain {1} has {2}", Line 256: ovfStoreDiskImage.getId(), Line 310: for (DiskImage ovfStoreDisk : ovfStoreDiskImages) { Line 311: boolean isBetterOvfDiskFound = false; Line 312: Line 313: // Check which disks are of OVF_STORE Line 314: String diskDecription = ovfStoreDisk.getDescription(); > this string is no longer relevant, same goes for the comment above it It doesn't really matters but I removed it Line 315: Map<String, Object> diskDescriptionMap; Line 316: try { Line 317: diskDescriptionMap = JsonHelper.jsonToMap(diskDecription); Line 318: } catch (IOException e) { -- 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: 6 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
