Maor Lipchuk has posted comments on this change. Change subject: core: Add alias and description for disk meta data ......................................................................
Patch Set 13: (4 comments) http://gerrit.ovirt.org/#/c/34163/13/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java: Line 354: Line 355: private void performDiskUpdate(final boolean unlockImage) { Line 356: if (isMetaDataPropertiesChangedForDiskImage()) { Line 357: updateMetaDataDescription((DiskImage) getNewDisk()); Line 358: } > This sentence doesn't compile. Can you elaborate? Lock in the Data base indicates an asynchronous task such as copy and move. A lock in the memory should be enough. I think that indicating locked status in the DB for disk when doing synchronous operation is not that practical, and might make the process more complex since we already doing synchronous operations.in the process.and also the disk is already locked in the memory Line 359: final Disk disk = getDiskDao().get(getParameters().getDiskId()); Line 360: applyUserChanges(disk); Line 361: Line 362: TransactionSupport.executeInNewTransaction(new TransactionMethod<Object>() { Line 401: && (!getOldDisk().getDiskAlias().equals(getNewDisk().getDiskAlias()) || !getOldDisk().getDiskDescription() Line 402: .equals(getNewDisk().getDiskDescription()))) { Line 403: return true; Line 404: } Line 405: return false; > We should not move the if condition to the ImagesHandler, I think that the Changed the if condition as suggested Line 406: } Line 407: Line 408: private void updateMetaDataDescription(DiskImage diskImage) { Line 409: try { Line 412: diskImage.getStorageIds().get(0), Line 413: diskImage.getId(), Line 414: diskImage.getImageId(), Line 415: getJsonDiskDescription()); Line 416: runVdsCommand(VDSCommandType.SetVolumeDescription, vdsCommandParameters); > I disagree, first the user can still encounter a failure when calling the V What I meant is that checking the Storage domain status before the VDS command only reduces the race, I will just add a validation in the CDA part and refactored it as suggested. Line 417: } catch (Exception e) { Line 418: StorageDomain storageDomain = Line 419: getStorageDomainDAO().getForStoragePool(diskImage.getStorageIds().get(0), Line 420: getVm().getStoragePoolId()); Line 430: try { Line 431: return ImagesHandler.getJsonDiskDescription(getParameters().getDiskInfo().getDiskAlias(), Line 432: getParameters().getDiskInfo().getDiskDescription()); Line 433: } catch (IOException e) { Line 434: throw new RuntimeException("Exception while generating json for disk", e); > I disagree, I think that this exception indicates that we failed on the jso Will change it and indicate the exception in line 418 Line 435: } Line 436: } Line 437: Line 438: protected void updateDiskProfile() { -- To view, visit http://gerrit.ovirt.org/34163 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie2642ae7016579ead699509426e01ac2010bd374 Gerrit-PatchSet: 13 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
