Liron Ar has posted comments on this change. Change subject: core: long query response time while many simultaneously queries are running ......................................................................
Patch Set 12: (3 comments) I still in favor of modifying the stored procedure as proposed instead of adding this part, please reconsider. I'm not blocking this change as it does improve things, but i think that the other approach would be preferable in all terms, please take a look http://gerrit.ovirt.org/#/c/27586/12/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsUpdateRunTimeInfo.java http://gerrit.ovirt.org/#/c/27586/12/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 108: Line 109: @Test Line 110: public void testGetImageGroupIdMapForImageList() { Line 111: Guid IMAGE_GROUP_ID = Guid.createGuidFromString("1b26a52b-b60f-44cb-9f46-3ef333b04a35"); Line 112: Guid IMAGE_ID_TO_GROUP = Guid.createGuidFromString("c9a559d9-8666-40d1-9967-759502b19f0d"); when you rebase if you could change the names to java standard it'll be great. Line 113: Line 114: Map<Guid, Guid> ids = dbFacade.getImageDao() Line 115: .getImageGroupIdMapForImageList(new HashSet<>(Arrays.asList(IMAGE_GROUP_ID))); Line 116: assertEquals(IMAGE_ID_TO_GROUP, ids.get(IMAGE_GROUP_ID)); http://gerrit.ovirt.org/#/c/27586/12/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() { I still think that the better approach is to change the stored procedure instead of adding this part - quoting from patch #10- that way we save that unnecessary loads the load is here just to perform the update, so we can easily encapsulate it within the update stored procedure which will improve things even more imo (less code added here, no need to add dedicated stored procedure, no need for this loop and generally better performance). I think that this approach has benefits, I'd suggest to reconsider it. Line 1722: if (_vdsManager.getRefreshStatistics()) { Line 1723: Set<Guid> requiredDisks = new HashSet<>(); Line 1724: for (VmInternalData vmInternalData : _runningVms.values()) { Line 1725: for (DiskImageDynamic disk : vmInternalData.getVmDynamic().getDisks()) { Line 1979: vmToUpdate.updateRunTimeStatisticsData(vmStatistics, vmToUpdate); Line 1980: addVmStatisticsToList(vmToUpdate.getStatisticsData()); Line 1981: updateInterfaceStatistics(vmToUpdate, vmStatistics); Line 1982: Line 1983: try { see related comment in patch #10, why is this try-catch block is needed as part of this change? Line 1984: for (DiskImageDynamic imageDynamic : _runningVms.get(vmToUpdate.getId()).getVmDynamic().getDisks()) { Line 1985: Guid activeImageId = diskImages.get(imageDynamic.getId()); Line 1986: // We have disk_id statistics, which is good, but disk_image_dynamic table contains image_id, so we Line 1987: // update for the AI. -- 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: 12 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
