Arik Hadas has posted comments on this change. Change subject: vdsbroker: reduced scope of synchronized blocks ......................................................................
Patch Set 1: (6 comments) I'm not sure about the change in IrsBrokerCommand, but regarding the change in VdsIdVDSCommandBase, it looks like it would not have a big impact: 1. the frequency of calls for these commands is expected to be low (right?) 2. it seems like the time in which the vds is locked is not significantly reduced If there is no obvious performance improvement, I would keep this lock in the base class for the sake of standardization. Piotr, is it possible that the improvement in the performance tests you ran results just from the removal of the lock in IrsBrokerCommand? https://gerrit.ovirt.org/#/c/37947/1/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/AddVdsVDSCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/AddVdsVDSCommand.java: Line 17: @Override Line 18: protected void executeVdsIdCommand() { Line 19: log.info("AddVds - entered , starting logic to add VDS '{}'", getVdsId()); Line 20: log.info("AddVds - VDS '{}' was added, will try to add it to the resource manager", Line 21: getVdsId()); do we need these log printings? VdsCommandBase#execute already logs it Line 22: synchronized (_vdsManager.getLockObj()) { Line 23: VDS vds = DbFacade.getInstance().getVdsDao().get(getVdsId()); Line 24: ResourceManager.getInstance().AddVds(vds, false); Line 25: } Line 22: synchronized (_vdsManager.getLockObj()) { Line 23: VDS vds = DbFacade.getInstance().getVdsDao().get(getVdsId()); Line 24: ResourceManager.getInstance().AddVds(vds, false); Line 25: } Line 26: } even if the logs above remain, isn't this change really-really negligible? https://gerrit.ovirt.org/#/c/37947/1/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/RemoveVdsVDSCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/RemoveVdsVDSCommand.java: Line 15: protected void executeVdsIdCommand() { Line 16: synchronized (_vdsManager.getLockObj()) { Line 17: ResourceManager.getInstance().RemoveVds(getVdsId(), parameters.isNewHost()); Line 18: } Line 19: } there is no difference here, right? https://gerrit.ovirt.org/#/c/37947/1/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/SetVdsStatusVDSCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/SetVdsStatusVDSCommand.java: Line 65: _vdsManager.updateStatisticsData(vds.getStatisticsData()); Line 66: return null; Line 67: } Line 68: }); Line 69: } since the call to getParameters and the 'if' doesn't involve call to the database or vdsm, isn't this change really really negligible? Line 70: } else { Line 71: getVDSReturnValue().setSucceeded(false); Line 72: } Line 73: } https://gerrit.ovirt.org/#/c/37947/1/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/UpdateVdsVMsClearedVDSCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/UpdateVdsVMsClearedVDSCommand.java: Line 16: getVds().setVmsCoresCount(0); Line 17: getVds().setVmActive(0); Line 18: getVds().setVmMigrating(0); Line 19: _vdsManager.updateDynamicData(getVds().getDynamicData()); Line 20: } again, since the 'if' check is quick, isn't this change really really negligible? Line 21: } else { Line 22: getVDSReturnValue().setSucceeded(false); Line 23: } Line 24: } https://gerrit.ovirt.org/#/c/37947/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 574: AuditLogType.VDS_FAILED_TO_RUN_VMS); Line 575: log.info("Vds '{}' moved to Error mode after {} attempts. Time: {}", vds.getName(), Line 576: mFailedToRunVmAttempts, new Date()); Line 577: } Line 578: } SetVdsStatus already locks the vds, why should be lock it when scheduling the quartz job? Line 579: } Line 580: Line 581: /** Line 582: */ -- To view, visit https://gerrit.ovirt.org/37947 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7d1bfd7b1fc7bfcc6465eae62feda6f1a27ff455 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Roy Golan <[email protected]> Gerrit-Reviewer: Tim Speetjens <[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
