Liron Ar has posted comments on this change. Change subject: core: long query response time while many simultaneously queries are running ......................................................................
Patch Set 10: Code-Review-1 (8 comments) marking -1 for visibility, seems like there's an issue with the query. 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 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. 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 1729 as well, as otherwise it's not used. 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 have their id's here only once. 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() 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? 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/dbscripts/images_sp.sql wrong results will be possibly returned for disks with snapshots. 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) { 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 2. seems that currently there's a bug here - in case of disks with snapshots for each image_group_id multiple rows will be returned while we need the active one (which will affect the engine flow), so you can should add the active column to the where clause as well. 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 <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Barak Azulay <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Liran Zelkha <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Roy Golan <[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
