Gilad Chaplik has posted comments on this change. Change subject: engine: support for adding network qos for rest-api ......................................................................
Patch Set 2: (17 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 g Done 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? Done 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? tried to refactor it a bit, it will make me do some nasty unsafe casting. prefer to leave it for now (or wait till Java have multiple inheritance) 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? same 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? don't remember :) I can go either way... 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? Done 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? Done 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? prefer to have it here, it's implemented once and prevent unsafe casting. 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 sit Done 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 coul good catch :) 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 remov I really don't understand it. 'qosDao.get' fetch qos element by qos id. in fixtures.xml the qos had the same id as iface (which is confusing). Don't know how it got deleted, but it shouldn't have (probably wrong delete cascade). removing the last line in method as well. 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 removing this entry, since currently it isn't supported (anonymous qos element) 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 (whi Done 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 Done 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. Done 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. Done Line 91: parameter.setQos(tempQos); Line 92: parameters.add(parameter); Line 93: } Line 94: Frontend.getInstance().runMultipleAction(VdcActionType.RemoveNetworkQoS, parameters); http://gerrit.ovirt.org/#/c/31868/2/packaging/dbscripts/upgrade/03_06_0320_refactor_network_qos.sql File packaging/dbscripts/upgrade/03_06_0320_refactor_network_qos.sql: Line 19: SELECT fn_db_drop_constraint('network', 'fk_network_qos_id'); Line 20: -- drop table Line 21: DROP TABLE network_qos; Line 22: -- create new references Line 23: SELECT fn_db_create_constraint('vnic_profiles', 'FK_vnic_profiles_network_qos_id', 'FOREIGN KEY (network_qos_id) REFERENCES qos(id) ON DELETE SET NULL'); > FK_vnic_profiles_network_qos_id => fK_vnic_profiles_network_qos_id Done Line 24: SELECT fn_db_create_constraint('network', 'fk_network_qos_id', 'FOREIGN KEY (qos_id) REFERENCES qos(id) ON DELETE SET NULL'); Line 25: -- allow for anonymous network QoS entities to be persisted, to be revisited. Line 26: ALTER TABLE qos Line 27: ALTER COLUMN name DROP NOT NULL, -- 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 <gchap...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@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
