Liron Ar has posted comments on this change. Change subject: db: aggregate qos and storage qos impl ......................................................................
Patch Set 6: (12 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 as well. 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 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> { > Done I'm not really in favor of having the base dao, for each usecase we can implement the needed stored procedure but i assume that we can change it later on if we'd want to. Line 15: private final QosType qosType; Line 16: RowMapper<T> mapper = createEntityRowMapper(); Line 17: Line 18: public BaseQosDaoFacadeImpl(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(); > > 2) should be protected, used in derived classes, don't see any reason to This mapper should be decalred as final as currently it isn't thread safe. also, it's more correct to have this field bound to the class and not to an instance (static), the fact that we have only one instance of this is "dao impl" class is an an implementation detail. 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 tend to agree with Moti and Yevgeny here. I'm also in favor of the suggested approach that is also more commonly used. Line 36: Line 37: /** Line 38: * @return qos type for derived qos dao Line 39: */ 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. 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 name. 2. no need for test prefix for the method name. Line 33: @Test Line 34: public void testGetStorageQos() { Line 35: StorageQos storageQos = new StorageQos(); Line 36: storageQos.setId(qosAId); Line 49: Line 50: /** Line 51: * test update Line 52: */ Line 53: @Test 1. i'd remove the comment, it doesn't provide more info than the method name. 2. no need for test prefix for the method name. Line 54: public void testUpdateStorageQos() { Line 55: StorageQos storageQos = new StorageQos(); Line 56: storageQos.setId(qosBId); Line 57: storageQos.setName("newB"); Line 79: } Line 80: Line 81: /** Line 82: * test save Line 83: */ 1. i'd remove the comment, it doesn't provide more info than the method name. 2. no need for test prefix for the method name. Line 84: @Test Line 85: public void testSaveStorageQos() { Line 86: StorageQos storageQos = new StorageQos(); Line 87: storageQos.setId(qosDId); 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 error. Line 100: } Line 101: Line 102: /** Line 103: * Test getAllForStoragePool Line 100: } Line 101: Line 102: /** Line 103: * Test getAllForStoragePool Line 104: */ 1. i'd remove the comment, it doesn't provide more info than the method name. 2. no need for test prefix for the method name. Line 105: @Test Line 106: public void testGetAllStorageQosForStoragePool() { Line 107: assertEquals(2, dao.getAllForStoragePoolId(FixturesTool.STORAGE_POOL_NFS_2).size()); Line 108: } 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 20: CONSTRAINT PK_qos_id PRIMARY KEY (id) 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 MATCH SIMPLE is the default IIRC, can be removed. Line 25: ON UPDATE NO ACTION ON DELETE CASCADE; 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 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; no need to specify the ON UPDATE clause compensation isn't used in the remove storage pool command on the storage pool object and when looking on the flow there when the pool is removed there shouldn't be any exception (that's also why the compensation isn't used on it) so at least currently that seems good enough without compensation, if it will be needed it can be done in a further patch imo. 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
