Yair Zaslavsky has posted comments on this change. Change subject: db: aggregate qos and storage qos impl ......................................................................
Patch Set 6: (8 comments) http://gerrit.ovirt.org/#/c/27094/6/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/qos/BaseQosDaoFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/qos/BaseQosDaoFacadeImpl.java: Line 12: import org.springframework.jdbc.core.namedparam.MapSqlParameterSource; Line 13: Line 14: public abstract class BaseQosDaoFacadeImpl<T extends BaseQos> extends DefaultGenericDaoDbFacade<T, Guid> implements QosDao<T> { Line 15: private final QosType qosType; Line 16: RowMapper<T> mapper = createEntityRowMapper(); > I fine with that. Why not? in addition, why isn't the member private? Line 17: Line 18: public BaseQosDaoFacadeImpl(QosType qosType) { Line 19: super("qos"); Line 20: this.qosType = qosType; http://gerrit.ovirt.org/#/c/27094/6/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/qos/StorageQosDaoTest.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/qos/StorageQosDaoTest.java: Line 12: public class StorageQosDaoTest extends BaseDAOTestCase { Line 13: Line 14: private final StorageQosDao dao = getDbFacade().getStorageQosDao(); Line 15: Line 16: private static final Guid qosAId = Guid.createGuidFromString("ae956031-6be2-43d6-bb90-5191c9253314"); Please use FixturesTool for constants. Line 17: private static final Guid qosBId = Guid.createGuidFromString("ae956031-6be2-43d6-bb90-5191c9253315"); Line 18: private static final Guid qosCId = Guid.createGuidFromString("ae956031-6be2-43d6-bb90-5191c9253316"); Line 19: private static final Guid qosDId = Guid.createGuidFromString("ae956031-6be2-43d6-bb90-5191c9253317"); Line 20: http://gerrit.ovirt.org/#/c/27094/6/backend/manager/modules/dal/src/test/resources/fixtures.xml File backend/manager/modules/dal/src/test/resources/fixtures.xml: Line 1070: <row> Line 1071: <value>ae956031-6be2-43d6-bb90-5191c9253314</value> Line 1072: <value>1</value> Line 1073: <value>qos_a</value> Line 1074: <value>You don't understand. There's relationship George, and then there's the George you know. Baudy George, Funny George</value> Good ones :) Line 1075: <value>386bffd1-e7ed-4b08-bce9-d7df10f8c9a0</value> Line 1076: <value>1000</value> Line 1077: <value>2000</value> Line 1078: <value>500</value> http://gerrit.ovirt.org/#/c/27094/6/packaging/dbscripts/qos_sp.sql File packaging/dbscripts/qos_sp.sql: Line 65: BEGIN Line 66: RETURN QUERY SELECT * Line 67: FROM qos Line 68: WHERE storage_pool_id = v_storage_pool_id Line 69: AND (v_qos_type IS NULL OR qos_type = v_qos_type); > not really. when i want all - i'd just call: +1 on Moti's comment. Line 70: END; $procedure$ Line 71: LANGUAGE plpgsql; Line 65: BEGIN Line 66: RETURN QUERY SELECT * Line 67: FROM qos Line 68: WHERE storage_pool_id = v_storage_pool_id Line 69: AND (v_qos_type IS NULL OR qos_type = v_qos_type); > the problem with that is that I don't have a DAO for all QoS. Instead we ca If you don't have a DAO for all QOS then you have a design flaw. XXXQosDao should not get QOS entities that are not XXX. You can consider having a stored procedure for store pool to get all QOS entities for it. Line 70: END; $procedure$ Line 71: LANGUAGE plpgsql; http://gerrit.ovirt.org/#/c/27094/6/packaging/dbscripts/upgrade/03_05_0330_qos_and_storage_impl.sql File packaging/dbscripts/upgrade/03_05_0330_qos_and_storage_impl.sql: Line 7: id uuid NOT NULL, Line 8: qos_type SMALLINT NOT NULL, Line 9: name VARCHAR(50) NOT NULL, Line 10: description TEXT, Line 11: storage_pool_id uuid NOT NULL, > please rename any references to storage_pool to data_center. Then there will be inconsistency with other db related scripts. Sad, I know. Worth asking on devel, if we haven't yet. Line 12: max_throughput INTEGER, Line 13: max_read_throughput INTEGER, Line 14: max_write_throughput INTEGER, Line 15: max_iops INTEGER, Line 21: ) WITH OIDS; Line 22: Line 23: ALTER TABLE qos ADD CONSTRAINT fk_qos_storage_pool FOREIGN KEY (storage_pool_id) Line 24: REFERENCES storage_pool (id) MATCH SIMPLE Line 25: ON UPDATE NO ACTION ON DELETE CASCADE; > @Eli, @Yair? Gilad - Moti is correct, RemoveStoragePool is a non transactional command. See how other entities are being removed. Line 26: Line 27: -- add index on storage_pool_id Line 28: CREATE INDEX IDX_qos_storage_pool_id ON qos (storage_pool_id); Line 29: Line 27: -- add index on storage_pool_id Line 28: CREATE INDEX IDX_qos_storage_pool_id ON qos (storage_pool_id); Line 29: Line 30: -- add index on qos_type Line 31: CREATE INDEX IDX_qos_type ON qos (qos_type); > and this is the exact reason why your index should look like: I agree with Moti here. Since the context of qos type is per data center, the index looks fine to me. -- To view, visit http://gerrit.ovirt.org/27094 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1a9af59277b5055453159f002f19046c0051d63b Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@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: Kobi Ianko <k...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@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
