Gilad Chaplik has posted comments on this change. Change subject: db: aggregate qos and storage qos impl ......................................................................
Patch Set 6: (14 comments) http://gerrit.ovirt.org/#/c/27094/6//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2014-04-29 13:34:30 +0300 Line 4: Commit: Kobi Ianko <[email protected]> Line 5: CommitDate: 2014-05-14 15:58:22 +0300 Line 6: Line 7: db: aggregate qos and storage qos impl > there is no 'db' component, please rename to 'engine' please 'git log | grep db:' and get back to me with your findings. I may consider renaming it to 'db, core' Line 8: Line 9: - DB and DAL changes: create qos table to accomodate qos objects and include Line 10: storage qos and its limits. Line 11: http://gerrit.ovirt.org/#/c/27094/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/ConfigurationValues.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/ConfigurationValues.java: Line 95: PreDefinedNetworkCustomProperties, Line 96: UserDefinedNetworkCustomProperties, Line 97: MultipleGatewaysSupported, Line 98: VirtIoScsiEnabled(ConfigAuthType.User), Line 99: HostNetworkQosSupported, > please move the Qos configuration values with the rest of the Qos. Done Line 100: StorageQosSupported, Line 101: SshSoftFencingCommand, Line 102: MemorySnapshotSupported(ConfigAuthType.User), Line 103: MaxAverageNetworkQoSValue, http://gerrit.ovirt.org/#/c/27094/6/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/DbFacade.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/DbFacade.java: Line 176: put(VnicProfile.class, VnicProfileDao.class); Line 177: put(VnicProfileView.class, VnicProfileDao.class); Line 178: put(DwhHistoryTimekeeping.class, DwhHistoryTimekeepingDao.class); Line 179: put(IscsiBond.class, IscsiBondDao.class); Line 180: put(VmInit.class, VmInitDAO.class); > since you should support applicative rollback, there should be a map betwee Done Line 181: } Line 182: }; Line 183: Line 184: private JdbcTemplate jdbcTemplate; 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 10: import org.ovirt.engine.core.dao.DefaultGenericDaoDbFacade; Line 11: import org.springframework.jdbc.core.RowMapper; 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> { > s/BaseQosDaoFacadeImpl/QosBaseDaoDbFacadeImpl Done Line 15: private final QosType qosType; Line 16: RowMapper<T> mapper = createEntityRowMapper(); Line 17: Line 18: public BaseQosDaoFacadeImpl(QosType qosType) { Line 21: } Line 22: Line 23: /** Line 24: * @param rs Line 25: * @return specific mapper for derived QoS dao > that's lie please calm down... Line 26: * @throws SQLException Line 27: */ Line 28: protected abstract T createPartialQosEntity(ResultSet rs) throws SQLException; Line 29: Line 31: * @param map Line 32: * @param obj Line 33: * @return parameters mapper for derived QoS dao Line 34: */ Line 35: protected abstract void updatePartialParametersMapper(MapSqlParameterSource map, T obj); > while updatePartialParametersMapper reduces the amount of code in the inher When we'll get to the bridge, we'll cross it. IMO, readability and # lines code, is much more important than perfect design, It's better to be smart than right. Line 36: Line 37: /** Line 38: * @return qos type for derived qos dao Line 39: */ Line 85: return getCustomMapSqlParameterSource() Line 86: .addValue("id", guid); Line 87: } Line 88: Line 89: protected Integer getIntegerOrNull(ResultSet rs, String columnName) throws SQLException { > In this case, please extract that method to BaseDAODbFacade (next to getLon will do it in vnic patch. don't want to fix it twice. Line 90: int i = rs.getInt(columnName); Line 91: return rs.wasNull() ? null : i; Line 92: } http://gerrit.ovirt.org/#/c/27094/6/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/qos/StorageQosDaoFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/qos/StorageQosDaoFacadeImpl.java: Line 6: import org.ovirt.engine.core.common.businessentities.qos.QosType; Line 7: import org.ovirt.engine.core.common.businessentities.qos.StorageQos; Line 8: import org.springframework.jdbc.core.namedparam.MapSqlParameterSource; Line 9: Line 10: public class StorageQosDaoFacadeImpl extends BaseQosDaoFacadeImpl<StorageQos> implements StorageQosDao { > Please rename the class to StorageQosDaoDbFacadeImpl Done Line 11: Line 12: public StorageQosDaoFacadeImpl() { Line 13: super(QosType.STORAGE); Line 14: } 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. Done 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/packaging/dbscripts/qos_sp.sql File packaging/dbscripts/qos_sp.sql: Line 1: ---------------------------------------------------------------- Line 2: -- [qos] Table Line 3: ---------------------------------------------------------------- Line 4: Line 5: Create or replace FUNCTION InsertQos(v_id uuid, > this func will not work if the call is missing some parameters, cpuQos does you will update the store-proc in your patch. Line 6: v_qos_type SMALLINT, Line 7: v_name VARCHAR(50), Line 8: v_description TEXT, Line 9: v_storage_pool_id uuid, 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); > I also agree with Moti's comments (will not write +1) Will open a code change bug. 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, > per @devel discussion, it was agreed that each component owner should take good to know, btw I'm the component owner of sla and scheduling. I will do it in a later patch for all my components. 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; > not validation, but compensation. currently validation. if you insist I'll open a bug for it. 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); > please extend IDX_qos_storage_pool_id to have also qos_type. @Eli, removing the index was already agreed. I prefer to stick with it. -- 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: Liron Ar <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: Yevgeny Zaspitsky <[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
