Michael Kublin has posted comments on this change. Change subject: engine: quota support for multiple storage images ......................................................................
Patch Set 9: I would prefer that you didn't submit this (6 inline comments) Sorry, I think that the whole concept of passing images from client to us, when we can easily retrieve images is wrong .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java Line 374: return QuotaManager.validateMultiStorageQuota(getStoragePool().getQuotaEnforcementType(), why u need a list for quota, and it is not needed for multiple storage domain feature? .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommonVmPoolWithVmsCommand.java Line 85: getStoragePool())); These is strange, a disks should be retrieved from Db, why we need that gui will send them to us. By the way, you know that size of imageToDestinationDomainMap can be different to size of you "strange" list .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommand.java Line 103: // Set default quota id if storage pool enforcement is disabled. these is happenning after constructor, so what for you are doing at constructor setQuotaId(getParameters().getQuotaId()); ? It is mean that after that a set value will be wrong? .................................................... File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/AddVmPoolWithVmsParameters.java Line 22: private ArrayList<DiskImage> diskInfoDestinationList; same here? what for? .................................................... File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/AddVmTemplateParameters.java Line 28: private HashMap<Guid, DiskImage> diskInfoDestinationMap; these is bad, why we need a DiskImage. It is means that gui/rest should retrieve a diskImage from us and send them again to us? What for ? Why diskId and quotaId is not enough? .................................................... File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VmManagementParametersBase.java Line 23: private ArrayList<DiskImage> diskInfoDestinationList; If you added a list now, u should delete existing one at all child classes, these is idea of inheritance -- To view, visit http://gerrit.ovirt.org/3090 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3e4b7e28fb9dd3069e86bd8fdf41255398fd1e58 Gerrit-PatchSet: 9 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Michael Kublin <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
