Martin Peřina has posted comments on this change. Change subject: core: FenceVdsVDSCommandParameters code cleanup ......................................................................
Patch Set 2: (3 comments) http://gerrit.ovirt.org/#/c/38061/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/FenceExecutor.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/FenceExecutor.java: Line 182: .RunVdsCommand( Line 183: VDSCommandType.FenceVds, Line 184: new FenceVdsVDSCommandParameters( Line 185: proxyHost.getId(), Line 186: _vds.getId(), > shouldn't we get rid of the name _vds and rename it according to Java notat I plan to replace FenceExecutor with FenceHostExecutor and FenceAgentExecutor in later patches, so I didn't play too much attention to FenceExecutor changes Line 187: agent, Line 188: action, Line 189: fencingPolicy)); Line 190: } http://gerrit.ovirt.org/#/c/38061/2/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/FenceVdsVDSCommandParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/FenceVdsVDSCommandParameters.java: Line 52: + "password = ***, options = '%s', policy = '%s'", Line 53: super.toString(), Line 54: getTargetVdsID(), Line 55: getAction(), Line 56: getFenceAgent().getIp(), > isn't it worth to call getFenceAgent once and set it to a "agent" variable Well, currently we don't have any global policy for entity object to contain meaningful toString() method (I mean that every entity object should contain toString() which will dump values of all its attributes). And IMHO this would be very useful for logging purposes. I will note this and add FenceAgent.toString() in some later patch Line 57: getFenceAgent().getPort(), Line 58: getFenceAgent().getType(), Line 59: getFenceAgent().getUser(), Line 60: getFenceAgent().getOptions(), http://gerrit.ovirt.org/#/c/38061/2/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/FenceVdsVDSCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/FenceVdsVDSCommand.java: Line 170: } Line 171: Line 172: protected FenceStatusReturnForXmlRpc fenceNode(FenceActionType actionType, boolean applyFencingPolicy) { Line 173: return getBroker().fenceNode( Line 174: getParameters().getFenceAgent().getIp(), > isn't it worth to set getParameters().getFenceAgent() to a variable named a Well, I'm not sure if it makes code more readable and/or faster ... Line 175: getParameters().getFenceAgent().getPort() == null Line 176: ? "" Line 177: : getParameters().getFenceAgent().getPort().toString(), Line 178: getParameters().getFenceAgent().getType(), -- To view, visit http://gerrit.ovirt.org/38061 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I055276a4012180fda23bb2063c38df3bdd6ce876 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
