Eli Mesika has posted comments on this change. Change subject: core: Eliminate unneeded updates to VdsDynamic ......................................................................
Patch Set 1: (3 comments) http://gerrit.ovirt.org/#/c/23852/1/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsDynamicDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsDynamicDAODbFacadeImpl.java: Line 267: return VdcDynamicRowMapper.instance; Line 268: } Line 269: Line 270: @Override Line 271: public void updateIfNeeded(VdsDynamic vdsDynamic) { Liran I am thinking what should the implementation be in order to easily extend it to other places One of my concerns is that IMHO usages should call only update and know nothing about the internal implementation that will check if update needed Other is that we have IIRC infrastructure hierarchy that might help us implement that in a higher level I think that the solution should be more general than that , lets discuss that offline Line 272: VdsDynamic dbData = get(vdsDynamic.getId()); Line 273: if (!dbData.equals(vdsDynamic)) { Line 274: update(vdsDynamic); Line 275: } else { Line 272: VdsDynamic dbData = get(vdsDynamic.getId()); Line 273: if (!dbData.equals(vdsDynamic)) { Line 274: update(vdsDynamic); Line 275: } else { Line 276: log.info("Ignored an unneeded update of VdsDynamic"); We won't want to see this in regular log , I think it should be a debug message Line 277: } Line 278: } http://gerrit.ovirt.org/#/c/23852/1/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsManager.java: Line 369: * Line 370: * @param dynamicData Line 371: */ Line 372: public void updateDynamicData(VdsDynamic dynamicData) { Line 373: DbFacade.getInstance().getVdsDynamicDao().updateIfNeeded(dynamicData); I think the call here should be to update, there are two cases 1) from the automatic update we surely want to update just as needed 2) from manual update we don't care to check since manual operations never cause a bottleneck in our system So, the question if to check if update needed is in the BE level and not in the code specific scenario Line 374: } Line 375: Line 376: /** Line 377: * Save statistics data to cache and DB. -- To view, visit http://gerrit.ovirt.org/23852 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icab3ed7d251573f150d28f966d0ad2363ecc3441 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liran Zelkha <[email protected]> Gerrit-Reviewer: Barak Azulay <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Liran Zelkha <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[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
