Moti Asayag has posted comments on this change. Change subject: db: aggregate qos and storage qos impl ......................................................................
Patch Set 6: Code-Review-1 (10 comments) 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. 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 between the Qos entities to their dao for the sake of compensation. 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 Line 15: private final QosType qosType; Line 16: RowMapper<T> mapper = createEntityRowMapper(); Line 17: Line 18: public BaseQosDaoFacadeImpl(QosType qosType) { 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 inheritors of BaseQosDaoFacadeImpl, it adds more complexity to the overall DAO design which already define and implement createEntityRowMapper() and createFullParametersMapper(). Suppose I'd like to extend StorageQosDaoDbFacadeImpl, which methods should I override ? updatePartialParametersMapper or the other mappers ? This makes the api less clear. As Yevgeny wrote before - the mapping methods which are defined as part of the DAO belongs to the Mapper. The DAO itself shouldn't care about ResultSet or any other implementation type which is encapsulated by the mapper. The proper design will be inheriting mappers from a base mapper. Line 36: Line 37: /** Line 38: * @return qos type for derived qos dao Line 39: */ Line 52: } Line 53: Line 54: @Override Line 55: protected RowMapper<T> createEntityRowMapper() { Line 56: return new RowMapper<T>() { > There is no reason for creating a Mapper instance on every call to the DAO. this is a collateral impact of the dao-mapper design mixture. Once separated properly, it could be easily achieved. Line 57: @Override Line 58: public T mapRow(ResultSet rs, int rowNum) Line 59: throws SQLException { Line 60: T entity = createPartialQosEntity(rs); Line 85: return getCustomMapSqlParameterSource() Line 86: .addValue("id", guid); Line 87: } Line 88: Line 89: protected Integer getIntegerOrNull(ResultSet rs, String columnName) throws SQLException { > NetworkQoSDaoFacadeImpl#21 In this case, please extract that method to BaseDAODbFacade (next to getLong) and reuse it instead of duplicating code. Also, please make it look alike getLong, i.e.: /** * Returns a Integer or a null if the column was NULL. * @param resultSet the ResultSet to extract the result from * @param columnName * @return a Integer or null * @throws SQLException */ final static Integer getInteger(ResultSet resultSet, String columnName) throws SQLException { if (resultSet.getIntcolumnName) == 0 && resultSet.wasNull()) { return null; } else { return resultSet.getLong(columnName); } } 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 The other reference got the same comment, but wasn't modified as asked to: http://gerrit.ovirt.org/#/c/16294/10/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/network/NetworkQoSDaoImpl.java,cm Line 11: Line 12: public StorageQosDaoFacadeImpl() { Line 13: super(QosType.STORAGE); Line 14: } 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, > @Yair, per @devel discussion, it was agreed that each component owner should take care of their code and enforce new named. Please use data_center instead of storage_pool 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; > okay will remove the cascade and add validation instead. not validation, but compensation. 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); > 1. Kobi, thank you for letting me learning something. i'm fine with either removing this index or extending IDX_qos_storage_pool_id to include qos_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: 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
