Ori Liel has posted comments on this change. Change subject: restapi: Host Power-Management Refactor (#977674) - WIP ......................................................................
Patch Set 34: (7 comments) http://gerrit.ovirt.org/#/c/27578/34/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddFenceAgentCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddFenceAgentCommand.java: Line 26: } Line 27: Line 28: @Override Line 29: protected boolean canDoAction() { Line 30: if (getParameters() == null || getParameters().getAgent() == null || getParameters().getAgent().getIp() == null > Could you please reformat to one condition per line staring with || operato Done Line 31: || getParameters().getAgent().getOrder() == null || getParameters().getAgent().getHostId() == null Line 32: || getParameters().getAgent().getPassword() == null || getParameters().getAgent().getType() == null Line 33: || getParameters().getAgent().getUser() == null) { Line 34: return failCanDoAction(VdcBllMessages.VDS_ADD_FENCE_AGENT_MANDATORY_PARAMETERS_MISSING); http://gerrit.ovirt.org/#/c/27578/34/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ProxyHostLocator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ProxyHostLocator.java: Line 18: import org.slf4j.Logger; Line 19: import org.slf4j.LoggerFactory; Line 20: import org.ovirt.engine.core.utils.pm.VdsFenceOptions; Line 21: Line 22: public class ProxyHostLocator { > Could you please rename it to FenceProxyLocator? So it would clearly say wh Done Line 23: Line 24: private static final Logger log = LoggerFactory.getLogger(ProxyHostLocator.class); Line 25: Line 26: private final VDS _vds; Line 99: Iterator<VDS> iterator = hosts.iterator(); Line 100: while (iterator.hasNext()) { Line 101: VDS host = iterator.next(); Line 102: if (host.getId().equals(_vds.getId()) || host.getId().equals(excludedHostId) Line 103: || !matchesOption(host, proxyOption) || !areAgentsSupported(host) || !isHostNetworkUnreacable(host)) { > This will filter out all hosts that has no networking problems Done Line 104: iterator.remove(); Line 105: } Line 106: } Line 107: for (VDS host : hosts) { Line 134: if (fencingPolicy != null) { Line 135: supported = Line 136: supported Line 137: && vds.getSupportedClusterVersionsSet() Line 138: .contains(FencingPolicyHelper.getMinimalSupportedVersion(fencingPolicy)); > Once again: please don't remove minVersionSupportingFencingPol (originally Done. I didn't want to make it a class member, because it's not a trait of the class, and also because I generally go with the Java principal: 'declared them when you need them'. However, I agree it's excessive to calculate it again for each host being checked, so I moved this check to the beginning of chooseBestProxy(). Line 139: } Line 140: return supported; Line 141: } Line 142: http://gerrit.ovirt.org/#/c/27578/34/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/FenceAgent.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/FenceAgent.java: Line 16: Line 17: private static final long serialVersionUID = -5910560758520427911L; Line 18: private Guid id; Line 19: private Guid hostId; Line 20: private Integer order; > 1. order should always be specified (even when only 1 fence agent exists) FenceAgent is a business-entity of the API layer and is generated from api.xsd. Entities aren't generated with any constructors, which means they only have the default empty constructor. This means that I can't enforce client code to set a value in 'order'. If order was a primitive (int) then when the engine receives a FenceAgent from the client with order=0, how can the engine know if client code explicitly set this field to 0, or simply ignored its existence, and it got the default value? For this reason I prefer to leave it an Integer. As for your second comment, firstly obviously I agree, but I'd like to point out that for small values (I think up to 256) a==b when 'a' and 'b' are Integers works exactly the same as a==b when 'a' and 'b' are primitives (int) - and we will never have more that 256 fence-agents for a host... Line 21: Line 22: @EditableField Line 23: private HashMap<String, String> optionsMap; Line 24: http://gerrit.ovirt.org/#/c/27578/34/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/VDSReturnValue.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/VDSReturnValue.java: Line 5: Line 6: public class VDSReturnValue { Line 7: Line 8: private boolean succeeded; Line 9: private String exceptionString = ""; > Please initialize instance variables only inside constructors Done Line 10: private Object returnValue; Line 11: private RuntimeException exceptionObject; Line 12: private AsyncTaskCreationInfo creationInfo; Line 13: private VDSError vdsError; http://gerrit.ovirt.org/#/c/27578/34/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsDAODbFacadeImpl.java: Line 31: /** Line 32: * <code>VdsDAODbFacadeImpl</code> provides an implementation of {@code VdsDAO} that uses previously written code from Line 33: * {@code DbFacade}. Line 34: */ Line 35: public class VdsDAODbFacadeImpl extends BaseDAODbFacade implements VdsDAO { > Is there any resolution to my comment for #18 (instead of adding another jo After having thought about it some more and consulting with Eli, I think it is possible to do what you suggest without creating problems in performance, and what you suggest is indeed the better solution. However, the patch's magnitude makes it vulnerable to changes and to 'noise' coming from more and more rebases, and since right now everything is tested and working well, we believe the best strategy is to merge it as is, and perform the change that you suggested in a separate patch. What do you say? Line 36: Line 37: @Override Line 38: public VDS get(Guid id) { Line 39: return get(id, null, false); -- To view, visit http://gerrit.ovirt.org/27578 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id0b384347a52db8b0aa6ae684fad40a5491dd2f7 Gerrit-PatchSet: 34 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ori Liel <ol...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Liran Zelkha <lzel...@redhat.com> Gerrit-Reviewer: Martin Peřina <mper...@redhat.com> Gerrit-Reviewer: Ori Liel <ol...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@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
