Amit Aviram has posted comments on this change. Change subject: core: Requesting to consume quota while extending a disk fix. ......................................................................
Patch Set 2: (5 comments) https://gerrit.ovirt.org/#/c/39330/2/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 689: boolean emptyOldQuota = oldDiskImage.getQuotaId() == null || Guid.Empty.equals(oldDiskImage.getQuotaId()); Line 690: boolean differentNewQuota = !emptyOldQuota && !oldDiskImage.getQuotaId().equals(newDiskImage.getQuotaId()); Line 691: long diskExtendingDiff = newDiskImage.getSizeInGigabytes() - oldDiskImage.getSizeInGigabytes(); Line 692: Line 693: if (emptyOldQuota || differentNewQuota ) { > I had some offline questions on this comments, so to clarify - I'm claiming Actually, if there is a new quota for a disk that didn't have quota, emptyOldQuota will be "true". this will make differentNewQuota to be false, and that is why emptyOldQuota is not redundant. on the other hand, if we are switching from an existing quota to another- than differentNewQuota will be true and emptyOldQuota false. So currently both are required.. Nevertheless, as this code is obviously not very readable- I think the best thing to do here is order it differently in another patch- first let's get this patch's logic to work.. Line 694: list.add(generateQuotaConsumeParameters(newDiskImage, newDiskImage.getSizeInGigabytes())); Line 695: } else if (diskExtendingDiff != 0) { Line 696: list.add(generateQuotaConsumeParameters(newDiskImage, diskExtendingDiff)); Line 697: } Line 691: long diskExtendingDiff = newDiskImage.getSizeInGigabytes() - oldDiskImage.getSizeInGigabytes(); Line 692: Line 693: if (emptyOldQuota || differentNewQuota ) { Line 694: list.add(generateQuotaConsumeParameters(newDiskImage, newDiskImage.getSizeInGigabytes())); Line 695: } else if (diskExtendingDiff != 0) { > Continuing Liron's comment from above, let's use ">" instead of "!=", just Done Line 696: list.add(generateQuotaConsumeParameters(newDiskImage, diskExtendingDiff)); Line 697: } Line 698: Line 699: if (differentNewQuota) { Line 692: Line 693: if (emptyOldQuota || differentNewQuota ) { Line 694: list.add(generateQuotaConsumeParameters(newDiskImage, newDiskImage.getSizeInGigabytes())); Line 695: } else if (diskExtendingDiff != 0) { Line 696: list.add(generateQuotaConsumeParameters(newDiskImage, diskExtendingDiff)); > Agreed, there might be a system wide problem on sending a quota with null i I will include it in the next patch as mentioned above. Don't want to mix different issues in one patch.. Line 697: } Line 698: Line 699: if (differentNewQuota) { Line 700: list.add(new QuotaStorageConsumptionParameter( https://gerrit.ovirt.org/#/c/39330/2/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateVmDiskCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateVmDiskCommandTest.java: Line 697: initializeCommand(parameters); Line 698: Line 699: QuotaStorageConsumptionParameter consumptionParameter = Line 700: (QuotaStorageConsumptionParameter) command.getQuotaStorageConsumptionParameters().get(0); Line 701: assertTrue(consumptionParameter.getRequestedStorageGB() == diskExtendingDiffInGB); > use assertEquals instead Done Line 702: } Line 703: Line 704: private long getBytesByGB(long sizeInGB) { Line 705: return sizeInGB*(1024*1024*1024); Line 701: assertTrue(consumptionParameter.getRequestedStorageGB() == diskExtendingDiffInGB); Line 702: } Line 703: Line 704: private long getBytesByGB(long sizeInGB) { Line 705: return sizeInGB*(1024*1024*1024); > Use org.ovirt.engine.core.common.utils.SizeConverter - its in the common pa Done Line 706: } Line 707: Line 708: private void mockToUpdateDiskVm(List<VM> vms) { Line 709: for (VM vm: vms) { -- To view, visit https://gerrit.ovirt.org/39330 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib31206b0a9172b35217845b01253951d93f380f1 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Amit Aviram <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Ala Hino <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Amit Aviram <[email protected]> Gerrit-Reviewer: Candace Sheremeta <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Freddy Rolland <[email protected]> Gerrit-Reviewer: Greg Padgett <[email protected]> Gerrit-Reviewer: Idan Shaby <[email protected]> Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Roy Golan <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: Vered Volansky <[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
