Gilad Chaplik has posted comments on this change. Change subject: db, core: aggregate qos and storage qos impl ......................................................................
Patch Set 15: (12 comments) http://gerrit.ovirt.org/#/c/27094/15/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 200: put(CommandEntity.class, CommandEntityDao.class); Line 201: put(ExternalVariable.class, ExternalVariableDao.class); Line 202: put(VdsKdumpStatus.class, VdsKdumpStatusDao.class); Line 203: put(VmJob.class, VmJobDao.class); Line 204: put(StorageQos.class, StorageQosDao.class); > see comment @ 7 Done Line 205: } Line 206: }; Line 207: Line 208: private JdbcTemplate jdbcTemplate; http://gerrit.ovirt.org/#/c/27094/15/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/qos/QosBaseDaoFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/qos/QosBaseDaoFacadeImpl.java: Line 63: return entity; Line 64: } Line 65: Line 66: /** Line 67: * @param rs > please document or remove Done Line 68: * @return entity of derived mapper class Line 69: * @throws SQLException Line 70: */ Line 71: protected abstract M createQosEntity(ResultSet rs) throws SQLException; Line 64: } Line 65: Line 66: /** Line 67: * @param rs Line 68: * @return entity of derived mapper class > the comment is wrong Done Line 69: * @throws SQLException Line 70: */ Line 71: protected abstract M createQosEntity(ResultSet rs) throws SQLException; Line 72: } http://gerrit.ovirt.org/#/c/27094/15/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/FixturesTool.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/FixturesTool.java: Line 612: Line 613: /** Line 614: * UUIDs for QoS objects Line 615: */ Line 616: public static final Guid QOS_A_UUID = new Guid("ae956031-6be2-43d6-bb90-5191c9253314"); > 1. /s/UUID/ID to match the convention is that file Done Line 617: Line 618: public static final Guid QOS_B_UUID = new Guid("ae956031-6be2-43d6-bb90-5191c9253315"); Line 619: Line 620: public static final Guid QOS_C_UUID = new Guid("ae956031-6be2-43d6-bb90-5191c9253316"); Line 618: public static final Guid QOS_B_UUID = new Guid("ae956031-6be2-43d6-bb90-5191c9253315"); Line 619: Line 620: public static final Guid QOS_C_UUID = new Guid("ae956031-6be2-43d6-bb90-5191c9253316"); Line 621: Line 622: public static final Guid QOS_D_UUID = new Guid("ae956031-6be2-43d6-bb90-5191c9253317"); > there's no record for that ID, so there's no need for the constant Done Line 623: /** Line 624: * Number of VMs on clusters Line 625: */ Line 626: public static final int NUMBER_OF_VMS_IN_VDS_GROUP_RHEL6_NFS_CLUSTER = 0; http://gerrit.ovirt.org/#/c/27094/15/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 43: assertEquals(storageQos, fetched); Line 44: } Line 45: Line 46: @Test Line 47: public void updateStorageQos() { > this test should be changed imo, it may be tested in less code. Done Line 48: StorageQos storageQos = new StorageQos(); Line 49: storageQos.setId(FixturesTool.QOS_B_UUID); Line 50: storageQos.setName("newB"); Line 51: storageQos.setDescription("If I owned a company, my employees would love me. They’d have huge pictures of me up the walls and in their home, like Lenin."); Line 63: assertEquals(storageQos, fetched); Line 64: } Line 65: Line 66: @Test Line 67: public void removeStorageQos() { > first get the entity and check that it's not null Done Line 68: dao.remove(FixturesTool.QOS_C_UUID); Line 69: assertNull(dao.get(FixturesTool.QOS_C_UUID)); Line 70: } Line 71: Line 71: Line 72: @Test Line 73: public void saveStorageQos() { Line 74: StorageQos storageQos = new StorageQos(); Line 75: storageQos.setId(FixturesTool.QOS_D_UUID); > let's generate here a new Guid, there's no need for a constant. Done Line 76: storageQos.setName("qos_d"); Line 77: storageQos.setDescription("bla bla"); Line 78: storageQos.setStoragePoolId(FixturesTool.STORAGE_POOL_MIXED_TYPES); Line 79: storageQos.setMaxThroughput(200); Line 81: storageQos.setMaxWriteThroughput(200); Line 82: storageQos.setMaxIops(200); Line 83: storageQos.setMaxReadIops(200); Line 84: storageQos.setMaxWriteIops(200); Line 85: > let's check at first that there's no record with the id that was mistakenly Done Line 86: dao.save(storageQos); Line 87: StorageQos fetched = dao.get(FixturesTool.QOS_D_UUID); Line 88: assertNotNull(fetched); Line 89: assertEquals(storageQos, fetched); http://gerrit.ovirt.org/#/c/27094/15/backend/manager/modules/dal/src/test/resources/fixtures.xml File backend/manager/modules/dal/src/test/resources/fixtures.xml: Line 1169: <value>1400</value> Line 1170: <value>500</value> Line 1171: </row> Line 1172: <row> Line 1173: <value>ba31682e-6ae7-4f9d-8c6f-04c93acca9db</value> > this recoed isn't used anywhere, please remove it Done Line 1174: <value>1</value> Line 1175: <value>qos_d</value> Line 1176: <value>Instead of doing a wash, I just keep buying underwear. My goal is to have over 360 pair. That way I only have to do wash once a year.</value> Line 1177: <value>72b9e200-f48b-4687-83f2-62828f249a47</value> http://gerrit.ovirt.org/#/c/27094/15/packaging/dbscripts/upgrade/03_05_0720_add_qos_table.sql File packaging/dbscripts/upgrade/03_05_0720_add_qos_table.sql: Line 24: REFERENCES storage_pool (id) 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); > wasn't it agreed to remove the indexes or to create one for both columns? it was agreed to remove the index (singular). and it was removed. http://gerrit.ovirt.org/#/c/27094/15/packaging/dbscripts/upgrade/pre_upgrade/0000_config.sql File packaging/dbscripts/upgrade/pre_upgrade/0000_config.sql: Line 228: select fn_db_add_config_value('StorageQosSupported','false','3.0'); Line 229: select fn_db_add_config_value('StorageQosSupported','false','3.1'); Line 230: select fn_db_add_config_value('StorageQosSupported','false','3.3'); Line 231: select fn_db_add_config_value('StorageQosSupported','false','3.4'); Line 232: select fn_db_add_config_value('StorageQosSupported','false','3.5'); > so it isn't supported for 3.5? :-) Done. Line 233: select fn_db_add_config_value('HostNetworkQosSupported', 'false', '3.0'); Line 234: select fn_db_add_config_value('HostNetworkQosSupported', 'false', '3.1'); Line 235: select fn_db_add_config_value('HostNetworkQosSupported', 'false', '3.2'); Line 236: select fn_db_add_config_value('HostNetworkQosSupported', 'false', '3.3'); -- 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: 15 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 Aravot <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[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
