Liran Zelkha has posted comments on this change. Change subject: core: long query response time while many simultaneously queries are running ......................................................................
Patch Set 10: -Verified (9 comments) http://gerrit.ovirt.org/#/c/27586/10/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageDaoDbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageDaoDbFacadeImpl.java: Line 133: } Line 134: return null; Line 135: } Line 136: Line 137: private static class ImageAndImageGroupId { > I'd use Pair<Guid,Guid> instead of adding this class Done Line 138: private Guid imageId; Line 139: private Guid imageGroupId; Line 140: Line 141: public Guid getImageId() { http://gerrit.ovirt.org/#/c/27586/10/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/ImageDaoTest.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/ImageDaoTest.java: Line 24: private static final Guid EXISTING_IMAGE_DISK_TEMPLATE_ID = new Guid("52058975-3d5e-484a-80c1-01c31207f578"); Line 25: private static final Guid EXISTING_SNAPSHOT_ID = new Guid("a7bb24df-9fdf-4bd6-b7a9-f5ce52da0f89"); Line 26: Line 27: private static final Guid IMAGE_GROUP_ID = Guid.createGuidFromString("1b26a52b-b60f-44cb-9f46-3ef333b04a35"); Line 28: private static final Guid IMAGE_ID_TO_GROUP = Guid.createGuidFromString("c9a559d9-8666-40d1-9967-759502b19f0d"); > those are used only for the added test, so i'd move it to there. Done Line 29: Line 30: private static final int TOTAL_IMAGES = 12; Line 31: private Image newImage; Line 32: http://gerrit.ovirt.org/#/c/27586/10/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java: Line 1717: // compare between vm in cache and vm from vdsm Line 1718: removeVmsFromCache(staleRunningVms); Line 1719: } Line 1720: Line 1721: private Map<Guid, Guid> loadImageGroupsIdsIfNeeded() { > the part between lines 1722-1727 should be inside the if clause in line 172 Done Line 1722: List<Guid> requiredDisks = new ArrayList<>(); Line 1723: for (VmInternalData vmInternalData : _runningVms.values()) { Line 1724: for (DiskImageDynamic disk : vmInternalData.getVmDynamic().getDisks()) { Line 1725: requiredDisks.add(disk.getId()); Line 1718: removeVmsFromCache(staleRunningVms); Line 1719: } Line 1720: Line 1721: private Map<Guid, Guid> loadImageGroupsIdsIfNeeded() { Line 1722: List<Guid> requiredDisks = new ArrayList<>(); > this should be a Set, if some vms has the same disks attached we should ha Done Line 1723: for (VmInternalData vmInternalData : _runningVms.values()) { Line 1724: for (DiskImageDynamic disk : vmInternalData.getVmDynamic().getDisks()) { Line 1725: requiredDisks.add(disk.getId()); Line 1726: } Line 1728: Line 1729: if (_vdsManager.getRefreshStatistics()) Line 1730: return getDbFacade().getImageDao().getImageGroupIdMapForImageList(requiredDisks); Line 1731: Line 1732: return new HashMap<>(); > return Collections.emptyMap() Done Line 1733: } Line 1734: Line 1735: private AuditLogType vmPauseStatusToAuditLogType(VmPauseStatus pauseStatus) { Line 1736: switch (pauseStatus) { Line 1977: vmToUpdate.updateRunTimeStatisticsData(vmStatistics, vmToUpdate); Line 1978: addVmStatisticsToList(vmToUpdate.getStatisticsData()); Line 1979: updateInterfaceStatistics(vmToUpdate, vmStatistics); Line 1980: Line 1981: try { > why is it needed as part of this change? This is where we use the diskImages we loaded earlier Line 1982: for (DiskImageDynamic imageDynamic : _runningVms.get(vmToUpdate.getId()).getVmDynamic().getDisks()) { Line 1983: Guid activeImageId = diskImages.get(imageDynamic.getId()); Line 1984: // We have disk_id statistics, which is good, but disk_image_dynamic table contains image_id, so we Line 1985: // update for the AI. Line 1979: updateInterfaceStatistics(vmToUpdate, vmStatistics); Line 1980: Line 1981: try { Line 1982: for (DiskImageDynamic imageDynamic : _runningVms.get(vmToUpdate.getId()).getVmDynamic().getDisks()) { Line 1983: Guid activeImageId = diskImages.get(imageDynamic.getId()); > see related comment here - http://gerrit.ovirt.org/#/c/27586/10/packaging/d Done Line 1984: // We have disk_id statistics, which is good, but disk_image_dynamic table contains image_id, so we Line 1985: // update for the AI. Line 1986: // We also check if the disk is null, as, for external VMs the disk is not in the database Line 1987: if (activeImageId != null) { Line 1980: Line 1981: try { Line 1982: for (DiskImageDynamic imageDynamic : _runningVms.get(vmToUpdate.getId()).getVmDynamic().getDisks()) { Line 1983: Guid activeImageId = diskImages.get(imageDynamic.getId()); Line 1984: // We have disk_id statistics, which is good, but disk_image_dynamic table contains image_id, so we > suggestion - instead of querying all the all the images just for the image If we use n as the number of updates, and m as the number of reads, we will still get n + m db commands, instead of n + 1. The fact that they are executed in a stored procedure still means they're happening... Line 1985: // update for the AI. Line 1986: // We also check if the disk is null, as, for external VMs the disk is not in the database Line 1987: if (activeImageId != null) { Line 1988: imageDynamic.setId(activeImageId); http://gerrit.ovirt.org/#/c/27586/10/packaging/dbscripts/images_sp.sql File packaging/dbscripts/images_sp.sql: Line 194: LANGUAGE plpgsql; Line 195: Line 196: DROP TYPE IF EXISTS image_image_group_rs CASCADE; Line 197: CREATE TYPE image_image_group_rs AS (image_guid UUID,image_group_id UUID); Line 198: > 1. /s/GetImageGroupByImageId/getImagesByImagesGroupId Done Line 199: Create or replace FUNCTION GetImageGroupByImageId(v_image_group_ids UUID[]) Line 200: RETURNS SETOF image_image_group_rs STABLE Line 201: AS $procedure$ Line 202: BEGIN -- To view, visit http://gerrit.ovirt.org/27586 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I94e20d782bc4e2befaf4338f51551a2855509769 Gerrit-PatchSet: 10 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liran Zelkha <lzel...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Liran Zelkha <lzel...@redhat.com> Gerrit-Reviewer: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@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
