Moti Asayag has posted comments on this change. Change subject: db: aggregate qos and storage qos impl ......................................................................
Patch Set 6: (7 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(); the convention is having the mapper as a static member of this mapper, per Dao. This will impose changes on the current design towards mapper hierarchy. Line 17: Line 18: public BaseQosDaoFacadeImpl(QosType qosType) { Line 19: super("qos"); Line 20: this.qosType = qosType; Line 73: obj reuse createIdParameterMapper(obj.getId()) which you defined next. Line 85: return getCustomMapSqlParameterSource() Line 86: .addValue("id", guid); Line 87: } Line 88: Line 89: protected Integer getIntegerOrNull(ResultSet rs, String columnName) throws SQLException { why this is required ? the control over the db population and querying is yours. Use: (Integer)rs.getObject(columnNameOfIntType). Line 90: int i = rs.getInt(columnName); Line 91: return rs.wasNull() ? null : i; Line 92: } 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); why v_qos_type should be null ? 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. (here, and elsewhere). StoragePool is an ancient notation. 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; cascade deletion is tricky - you should handle compensation in RemoveStoragePoolCommand, so in case of a rollback the qos will be restored. 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); i don't think this is a valid index. You only expose s/p for selecting by data-center-id, and the QoS entity has no meaning when not associated with a data-center. I'd suggest dropping this index and updating IDX_qos_storage_pool_id to include both dc-id and type. -- 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 <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Doron Fediuck <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Kobi Ianko <[email protected]> Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Moti Asayag <[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
