Lior Vernia has posted comments on this change. Change subject: engine: support for adding network qos for rest-api ......................................................................
Patch Set 2: (16 comments) http://gerrit.ovirt.org/#/c/31868/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/qos/AddNetworkQoSCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/qos/AddNetworkQoSCommand.java: Line 7: import org.ovirt.engine.core.common.action.QosParametersBase; Line 8: import org.ovirt.engine.core.common.businessentities.network.NetworkQoS; Line 9: import org.ovirt.engine.core.dao.qos.QosDao; Line 10: Line 11: public class AddNetworkQoSCommand extends AddQosCommand<NetworkQoS, QosValidator<NetworkQoS>> { This should be NetworkQosValidator to be consistent with your override of getQosValidator(). Line 12: Line 13: public AddNetworkQoSCommand(QosParametersBase<org.ovirt.engine.core.common.businessentities.network.NetworkQoS> parameters) { Line 14: super(parameters); Line 15: } Line 9: import org.ovirt.engine.core.dao.qos.QosDao; Line 10: Line 11: public class AddNetworkQoSCommand extends AddQosCommand<NetworkQoS, QosValidator<NetworkQoS>> { Line 12: Line 13: public AddNetworkQoSCommand(QosParametersBase<org.ovirt.engine.core.common.businessentities.network.NetworkQoS> parameters) { Surely you don't need the full class path? Line 14: super(parameters); Line 15: } Line 16: Line 17: @Override Line 14: super(parameters); Line 15: } Line 16: Line 17: @Override Line 18: protected QosDao<NetworkQoS> getQosDao() { Why don't you implement this in QosCommandBase similarly to QosQueryBase? Line 19: return getDbFacade().getNetworkQosDao(); Line 20: } Line 21: Line 22: @Override Line 19: return getDbFacade().getNetworkQosDao(); Line 20: } Line 21: Line 22: @Override Line 23: protected NetworkQosValidator getQosValidator(NetworkQoS networkQos) { This could be similarly implemented in the base class, couldn't it? Line 24: return new NetworkQosValidator(networkQos); Line 25: } Line 26: Line 27: @Override http://gerrit.ovirt.org/#/c/31868/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/qos/RemoveNetworkQoSCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/qos/RemoveNetworkQoSCommand.java: Line 14 Line 15 Line 16 Line 17 Line 18 Did you remove the consistentDataCenter() check on purpose? Line 6: import org.ovirt.engine.core.common.action.QosParametersBase; Line 7: import org.ovirt.engine.core.common.businessentities.network.NetworkQoS; Line 8: import org.ovirt.engine.core.dao.qos.QosDao; Line 9: Line 10: public class RemoveNetworkQoSCommand extends RemoveQosCommandBase<NetworkQoS, QosValidator<NetworkQoS>> { NetworkQosValidator for consistency? Line 11: Line 12: public RemoveNetworkQoSCommand(QosParametersBase<NetworkQoS> parameters) { Line 13: super(parameters); Line 14: } http://gerrit.ovirt.org/#/c/31868/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/qos/UpdateNetworkQoSCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/qos/UpdateNetworkQoSCommand.java: Line 7: import org.ovirt.engine.core.common.action.QosParametersBase; Line 8: import org.ovirt.engine.core.common.businessentities.network.NetworkQoS; Line 9: import org.ovirt.engine.core.dao.qos.QosDao; Line 10: Line 11: public class UpdateNetworkQoSCommand extends UpdateQosCommandBase<NetworkQoS, QosValidator<NetworkQoS>> { NetworkQosValidator for consistency? Line 12: Line 13: public UpdateNetworkQoSCommand(QosParametersBase<NetworkQoS> parameters) { Line 14: super(parameters); Line 15: } http://gerrit.ovirt.org/#/c/31868/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/NetworkQosValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/NetworkQosValidator.java: Line 42: return peak != null && peak < average; Line 43: } Line 44: Line 45: @Override Line 46: protected QosDao<NetworkQoS> getQosDao() { Why not implement this in QosValidator, similarly to QosQueryBase? Line 47: return DbFacade.getInstance().getNetworkQosDao(); Line 48: } Line 49: http://gerrit.ovirt.org/#/c/31868/2/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/network/NetworkQoS.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/network/NetworkQoS.java: Line 87: this.outboundBurst = outboundBurst; Line 88: } Line 89: Line 90: @Override Line 91: public boolean equalValues(QosBase obj) { This is not the right signature for the method - you shouldn't get to a situation where you compare values with an entity that is of a different type. In fact, I don't see any reason why you have the equalValues() method in QosBase - it isn't called anywhere in a polymorphic way (nor should it be). Line 92: if (!(obj instanceof NetworkQoS)) { Line 93: return false; Line 94: } Line 95: NetworkQoS other = (NetworkQoS) obj; Line 140: Line 141: @Override Line 142: public int hashCode() { Line 143: final int prime = 31; Line 144: int result = 1; The result should probably be based on super.hashCode(), otherwise you could have two distinct QoS entities seem like they're the same one just because they have the same values. Line 145: result = prime * result + ((inboundAverage == null) ? 0 : inboundAverage.hashCode()); Line 146: result = prime * result + ((inboundPeak == null) ? 0 : inboundPeak.hashCode()); Line 147: result = prime * result + ((inboundBurst == null) ? 0 : inboundBurst.hashCode()); Line 148: result = prime * result + ((outboundAverage == null) ? 0 : outboundAverage.hashCode()); http://gerrit.ovirt.org/#/c/31868/2/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/network/InterfaceDaoTest.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/network/InterfaceDaoTest.java: Line 122 Line 123 Line 124 Line 125 Line 126 Why did you remove this test? It's important to see that prior to the removal the interface had QoS reported on it (otherwise the later check doesn't mean much). http://gerrit.ovirt.org/#/c/31868/2/backend/manager/modules/dal/src/test/resources/fixtures.xml File backend/manager/modules/dal/src/test/resources/fixtures.xml: Line 1353: <value>1400</value> Line 1354: <value>500</value> Line 1355: </row> Line 1356: <row> Line 1357: <value>da31682e-6ae7-4f9d-8c6f-04c93acca9db</value> What is this ID? It looks like it has no meaning. Please keep the previous ID, which refers to an existing host NIC. You need this for the test (that you removed) to pass. Line 1358: <value>3</value> Line 1359: <null /> Line 1360: <null /> Line 1361: <null /> http://gerrit.ovirt.org/#/c/31868/2/backend/manager/modules/vdsbroker/src/test/java/org/ovirt/engine/core/vdsbroker/vdsbroker/SetupNetworksVDSCommandTest.java File backend/manager/modules/vdsbroker/src/test/java/org/ovirt/engine/core/vdsbroker/vdsbroker/SetupNetworksVDSCommandTest.java: Line 186: verifyMethodPassedToHost(); Line 187: Map<String, Object> networkStruct = assertNeworkWasSent(network); Line 188: NetworkQosMapper qosMapper = Line 189: new NetworkQosMapper(networkStruct, VdsProperties.HOST_QOS_INBOUND, VdsProperties.HOST_QOS_OUTBOUND); Line 190: NetworkQoS deserialize = qosMapper.deserialize(); This "correction" is wrong, as vdsm would never report QoS with the ID (which is a pure implementation detail of the engine). You may assert that they're equal according to their values instead. Line 191: if (deserialize != null) { Line 192: deserialize.setId(expectedQos.getId()); Line 193: } Line 194: assertEquals(expectedQos, deserialize); http://gerrit.ovirt.org/#/c/31868/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/EditNetworkQoSModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/EditNetworkQoSModel.java: Line 25: Line 26: @Override Line 27: public void executeSave() { Line 28: QosParametersBase<NetworkQoS> parameters = new QosParametersBase<NetworkQoS>(); Line 29: parameters.setQosId(networkQoS.getId()); Why would you ever need to set the ID separately from the QoS when the QoS entity contains its ID? Line 30: parameters.setQos(networkQoS); Line 31: Frontend.getInstance().runAction(VdcActionType.UpdateNetworkQoS, parameters, new IFrontendActionAsyncCallback() { Line 32: @Override Line 33: public void executed(FrontendActionAsyncResult result1) { http://gerrit.ovirt.org/#/c/31868/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/NewNetworkQoSModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/NewNetworkQoSModel.java: Line 25: @Override Line 26: protected void executeSave() { Line 27: // New network QoS Line 28: final QosParametersBase<NetworkQoS> parameters = new QosParametersBase<NetworkQoS>(); Line 29: parameters.setQosId(networkQoS.getId()); Same comment as in the edit model. Line 30: parameters.setQos(networkQoS); Line 31: Frontend.getInstance().runAction(VdcActionType.AddNetworkQoS, parameters, new IFrontendActionAsyncCallback() { Line 32: @Override Line 33: public void executed(FrontendActionAsyncResult result1) { http://gerrit.ovirt.org/#/c/31868/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/RemoveNetworkQoSModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/RemoveNetworkQoSModel.java: Line 86: Line 87: for (Object networkQoS : sourceListModel.getSelectedItems()) { Line 88: QosParametersBase<NetworkQoS> parameter = new QosParametersBase<NetworkQoS>(); Line 89: NetworkQoS tempQos = (NetworkQoS) networkQoS; Line 90: parameter.setQosId(tempQos.getId()); Same comment as in the previous two models. Line 91: parameter.setQos(tempQos); Line 92: parameters.add(parameter); Line 93: } Line 94: Frontend.getInstance().runMultipleAction(VdcActionType.RemoveNetworkQoS, parameters); -- To view, visit http://gerrit.ovirt.org/31868 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I34f43f9edc10b7b52e096b2b6f1f43d19f129ed5 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Doron Fediuck <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Lior Vernia <[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
