Eli Mesika has posted comments on this change. Change subject: core: Remove error messages from log when adding host ......................................................................
Patch Set 1: (9 comments) http://gerrit.ovirt.org/#/c/24980/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsCommand.java: Line 51: protected void initializeVds() { Line 52: initializeVds(true); Line 53: } Line 54: Line 55: protected void initializeVds(boolean expectHostToBeFound) { I would change the name of the flag to isNewHost and revert the logic in the RemoveVds Line 56: Backend.getInstance() Line 57: .getResourceManager() Line 58: .RunVdsCommand(VDSCommandType.RemoveVds, Line 59: new RemoveVdsVDSCommandParameters(getVdsId(), expectHostToBeFound)); http://gerrit.ovirt.org/#/c/24980/1/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/RemoveVdsVDSCommandParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/RemoveVdsVDSCommandParameters.java: Line 11: Line 12: public RemoveVdsVDSCommandParameters() { Line 13: } Line 14: Line 15: public RemoveVdsVDSCommandParameters(Guid vdsId, boolean errorIfHostDoesntExist) { errorIfHostDoesntExist => isNewHost Line 16: super(vdsId); Line 17: this.errorIfHostDoesntExist = errorIfHostDoesntExist; Line 18: } Line 19: http://gerrit.ovirt.org/#/c/24980/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 7: import org.ovirt.engine.core.utils.log.LogFactory; Line 8: Line 9: public class AddVdsVDSCommand<P extends AddVdsVDSCommandParameters> extends VdsIdVDSCommandBase<P> { Line 10: public AddVdsVDSCommand(P parameters) { Line 11: super(parameters, false); false=>true (after changing to isNewHost) Line 12: } Line 13: Line 14: @Override Line 15: protected void executeVdsIdCommand() { http://gerrit.ovirt.org/#/c/24980/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 6: Line 7: private P parameters; Line 8: Line 9: public RemoveVdsVDSCommand(P parameters) { Line 10: super(parameters, parameters.isErrorIfHostDoesntExist()); isErrorIfHostDoesntExist => isNewHost Line 11: this.parameters = parameters; Line 12: } Line 13: Line 14: @Override http://gerrit.ovirt.org/#/c/24980/1/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/ResourceManager.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/ResourceManager.java: Line 223: } Line 224: } Line 225: Line 226: public VdsManager GetVdsManager(Guid vdsId) { Line 227: return GetVdsManager(vdsId, true); true=>false Line 228: } Line 229: Line 230: public VdsManager GetVdsManager(Guid vdsId, boolean errorIfManagerNotFound) { Line 231: VdsManager vdsManger = vdsManagersDict.get(vdsId); Line 229: Line 230: public VdsManager GetVdsManager(Guid vdsId, boolean errorIfManagerNotFound) { Line 231: VdsManager vdsManger = vdsManagersDict.get(vdsId); Line 232: if (vdsManger == null) { Line 233: if (errorIfManagerNotFound) { if (! isNewHost) Line 234: log.errorFormat("Cannot get vdsManager for vdsid={0}", vdsId); Line 235: } else { Line 236: log.infoFormat("Cannot get vdsManager for vdsid={0}", vdsId); Line 237: } Line 232: if (vdsManger == null) { Line 233: if (errorIfManagerNotFound) { Line 234: log.errorFormat("Cannot get vdsManager for vdsid={0}", vdsId); Line 235: } else { Line 236: log.infoFormat("Cannot get vdsManager for vdsid={0}", vdsId); I don't think we have to log that in this case at all , its a new host .... Line 237: } Line 238: } Line 239: return vdsManger; Line 240: } http://gerrit.ovirt.org/#/c/24980/1/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsIdVDSCommandBase.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VdsIdVDSCommandBase.java: Line 9: protected VdsManager _vdsManager; Line 10: Line 11: public VdsIdVDSCommandBase(P parameters, boolean expectVdsManager) { Line 12: super(parameters); Line 13: _vdsManager = ResourceManager.getInstance().GetVdsManager(parameters.getVdsId(), expectVdsManager); IIUC the result of that is null if the flag is false , so why not simply wrap this setting with an if on the flag value , it is more clear IMO Line 14: } Line 15: Line 16: public VdsIdVDSCommandBase(P parameters) { Line 17: this(parameters, true); Line 13: _vdsManager = ResourceManager.getInstance().GetVdsManager(parameters.getVdsId(), expectVdsManager); Line 14: } Line 15: Line 16: public VdsIdVDSCommandBase(P parameters) { Line 17: this(parameters, true); true=>false Line 18: } Line 19: Line 20: protected Guid getVdsId() { Line 21: return getParameters().getVdsId(); -- To view, visit http://gerrit.ovirt.org/24980 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia17656dbfa0e16457565b04b849eb80ea74a8949 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: Martin Peřina <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[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
