Piotr Kliczewski has posted comments on this change. Change subject: vdsbroker: reduced scope of synchronized blocks ......................................................................
Patch Set 1: (6 comments) We saw that one of the customer (3.4) had issues with request timeout. The requests were not send to vdsm but the timeout occurred. I requested thread dump and saw 80% of threads being blocked. Yuri and Tim run the same flow for 3.5 and we noticed 2 places in the code that block threads. - irsbroker - storage tasks are already serialized on vdsm side so there is no reason to have it in 2 places - VdsIdVDSCommandBase - there were few threads blocked on it as well so I have decided to reduce synchronization scope. The changes were perf tested and we saw no blocked threads. 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 I did not want to change existing behavior but if you think we should remove it will do 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? When you look at other commands there is a value in moving synchronized down the stack. Not really for this command but for others. 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? When you look at other commands there is a value in moving synchronized down the stack. Not really for this command but for others. 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 da As stated before what we want to achieve is reduce the scope of synchronization. In perf testing we saw number of threads being blocked on LockObj. 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 negli As stated before. 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 t Please take a look at 557. It was synchronized on vdsManager#lockObj so we want to keep it. 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
