Gilad Chaplik has posted comments on this change. Change subject: db: aggregate qos and storage qos impl ......................................................................
Patch Set 6: -Verified (9 comments) http://gerrit.ovirt.org/#/c/27094/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/qos/BaseQos.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/qos/BaseQos.java: Line 26: @NotNull(message = "QOS_NAME_NOT_NULL") Line 27: @Size(min = 1, max = BusinessEntitiesDefinitions.GENERAL_NAME_SIZE, message = "QOS_NAME_TOO_LONG") Line 28: @ValidI18NName(message = "QOS_NAME_INVALID") Line 29: private String name; Line 30: private String description; > if we already have validation for all the fields, we can have one for that Done Line 31: Line 32: private BaseQos() { Line 33: } Line 34: 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(); > This mapper should be decalred as final as currently it isn't thread safe. implementation detail is also writing in Java :-) Done. Line 17: Line 18: public BaseQosDaoFacadeImpl(QosType qosType) { Line 19: super("qos"); Line 20: this.qosType = qosType; 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'd make the member private and add a protected getter, but I guess it's a Done Line 17: Line 18: public BaseQosDaoFacadeImpl(QosType qosType) { Line 19: super("qos"); Line 20: this.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); > I'm also in favor of the suggested approach that is also more commonly used Done Line 36: Line 37: /** Line 38: * @return qos type for derived qos dao Line 39: */ Line 83: @Override Line 84: protected MapSqlParameterSource createIdParameterMapper(Guid guid) { Line 85: return getCustomMapSqlParameterSource() Line 86: .addValue("id", guid); Line 87: } > Can you use org.ovirt.engine.core.dao.BaseDAODbFacade#createGuidMapper ? Done Line 88: Line 89: protected Integer getIntegerOrNull(ResultSet rs, String columnName) throws SQLException { Line 90: int i = rs.getInt(columnName); Line 91: return rs.wasNull() ? null : i; http://gerrit.ovirt.org/#/c/27094/6/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/qos/StorageQosDao.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/qos/StorageQosDao.java: Line 1: package org.ovirt.engine.core.dao.qos; Line 2: Line 3: import org.ovirt.engine.core.common.businessentities.qos.StorageQos; Line 4: Line 5: public interface StorageQosDao extends QosDao<StorageQos> { > i'd add it when it's needed. it's needed now. 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 28: } Line 29: Line 30: /** Line 31: * Ensures that retrieving VDS by ID works as expected. Line 32: */ > 1. i'd remove the comment, it doesn't provide more info than the method nam 1. it doesn't do any harm 2. explain? Line 33: @Test Line 34: public void testGetStorageQos() { Line 35: StorageQos storageQos = new StorageQos(); Line 36: storageQos.setId(qosAId); Line 95: storageQos.setMaxReadIops(200); Line 96: storageQos.setMaxWriteIops(200); Line 97: Line 98: dao.save(storageQos); Line 99: assertEquals(storageQos, dao.get(qosDId)); > i'd added assetNotNull on the received entity first to have more specific e Done Line 100: } Line 101: Line 102: /** Line 103: * Test getAllForStoragePool 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 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; > All it takes is an exception in freeMacs(), which seems to occur and trigge I will open a bug for it... looks like a deeper issue. 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: -- 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
