Allon Mureinik has posted comments on this change. Change subject: core: Quota consumption for Vm's disk updating refactor. ......................................................................
Patch Set 1: Code-Review-1 (5 comments) See inline. Also, are there tests covering all of this (not only extending a disk, that was covered in the previous patch)? If not, let's add them. https://gerrit.ovirt.org/#/c/39398/1/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 686: DiskImage oldDiskImage = (DiskImage) getOldDisk(); Line 687: DiskImage newDiskImage = (DiskImage) getNewDisk(); Line 688: Line 689: boolean doesNewDiskHaveQuota = isDiskImageHasQuota(newDiskImage); Line 690: boolean doesOldDiskHadQuota = isDiskImageHasQuota(oldDiskImage); These two are redundant - see below. Line 691: boolean isQuotaChanged = !oldDiskImage.getQuotaId().equals(newDiskImage.getQuotaId()); Line 692: long diskExtendingDiff = newDiskImage.getSizeInGigabytes() - oldDiskImage.getSizeInGigabytes(); Line 693: Line 694: // Generate a CONSUME request Line 687: DiskImage newDiskImage = (DiskImage) getNewDisk(); Line 688: Line 689: boolean doesNewDiskHaveQuota = isDiskImageHasQuota(newDiskImage); Line 690: boolean doesOldDiskHadQuota = isDiskImageHasQuota(oldDiskImage); Line 691: boolean isQuotaChanged = !oldDiskImage.getQuotaId().equals(newDiskImage.getQuotaId()); Can the old quota be null? Offhand, seems it can't - if this is incorrect, you should use Objects.equals Line 692: long diskExtendingDiff = newDiskImage.getSizeInGigabytes() - oldDiskImage.getSizeInGigabytes(); Line 693: Line 694: // Generate a CONSUME request Line 695: if (doesNewDiskHaveQuota) { Line 691: boolean isQuotaChanged = !oldDiskImage.getQuotaId().equals(newDiskImage.getQuotaId()); Line 692: long diskExtendingDiff = newDiskImage.getSizeInGigabytes() - oldDiskImage.getSizeInGigabytes(); Line 693: Line 694: // Generate a CONSUME request Line 695: if (doesNewDiskHaveQuota) { This is redundant - let the underlying infra handle it. And in any event, the RELEASE param should be under this check Line 696: if (isQuotaChanged) { Line 697: list.add(generateQuotaRequestParameters(newDiskImage, Line 698: QuotaConsumptionParameter.QuotaAction.CONSUME, Line 699: newDiskImage.getSizeInGigabytes())); Line 704: } Line 705: } Line 706: Line 707: // Generate a RELEASE request Line 708: if (doesOldDiskHadQuota && isQuotaChanged) { doesOldDiskHadQuota is redundant - just let the infra handle it. Line 709: list.add(generateQuotaRequestParameters(oldDiskImage, Line 710: QuotaConsumptionParameter.QuotaAction.RELEASE, Line 711: oldDiskImage.getSizeInGigabytes())); Line 712: } Line 725: (double) sizeInGigabytes ); Line 726: } Line 727: Line 728: Line 729: private boolean isDiskImageHasQuota(DiskImage diskImage) { Once you clear up the getQuotaStorageConsumptionParameters, this will be redundant. Line 730: return !(diskImage.getQuotaId() == null) && !Guid.Empty.equals(diskImage.getQuotaId()); Line 731: } Line 732: Line 733: private boolean resizeDiskImageRequested() { -- To view, visit https://gerrit.ovirt.org/39398 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I838973e9f1e71d36193fb2ea832b18b9308c7248 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Amit Aviram <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Amit Aviram <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Freddy Rolland <[email protected]> Gerrit-Reviewer: Idan Shaby <[email protected]> Gerrit-Reviewer: Liron Aravot <[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
