Liron Ar has posted comments on this change. Change subject: db: aggregate qos and storage qos impl ......................................................................
Patch Set 7: (8 comments) http://gerrit.ovirt.org/#/c/27094/7/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 188: put(VmInit.class, VmInitDAO.class); Line 189: put(CpuStatistics.class, VdsCpuStatisticsDAO.class); Line 190: put(VdsNumaNode.class, VdsNumaNodeDAO.class); Line 191: put(VmNumaNode.class, VmNumaNodeDAO.class); Line 192: put(StorageQos.class, StorageQosDao.class); this map is used for compensation, as currently we don't use it for qos , it's unneeded. Line 193: } Line 194: }; Line 195: Line 196: private JdbcTemplate jdbcTemplate; http://gerrit.ovirt.org/#/c/27094/7/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/BaseDAODbFacade.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/BaseDAODbFacade.java: Line 129: * @param columnName Line 130: * @return a Integer or null Line 131: * @throws SQLException Line 132: */ Line 133: protected final static Integer getInteger(ResultSet resultSet, String columnName) throws SQLException { I'd change this method implementation to: Integer a = resultSet.getInt(columnName); return resultSet.wasNull? null : a; Line 134: if (resultSet.getInt(columnName) == 0 && resultSet.wasNull()) { Line 135: return null; Line 136: } else { Line 137: return resultSet.getInt(columnName); http://gerrit.ovirt.org/#/c/27094/7/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 89: assertEquals(storageQos, fetched); Line 90: } Line 91: Line 92: @Test Line 93: public void getAllStorageQosForStoragePool() { > This will return false once there will be more types of qos on that storage it shouldn't, as the call for the sp from the dao adds the type. Line 94: List<StorageQos> allForStoragePoolId = dao.getAllForStoragePoolId(FixturesTool.STORAGE_POOL_MIXED_TYPES); Line 95: assertNotNull(allForStoragePoolId); Line 96: assertEquals(2, allForStoragePoolId.size()); Line 97: } http://gerrit.ovirt.org/#/c/27094/7/packaging/dbscripts/upgrade/03_05_0330_qos_and_storage_impl.sql File packaging/dbscripts/upgrade/03_05_0330_qos_and_storage_impl.sql: Line 16: max_read_iops INTEGER, Line 17: max_write_iops INTEGER, Line 18: _create_date TIMESTAMP WITH TIME ZONE default LOCALTIMESTAMP, Line 19: _update_date TIMESTAMP WITH TIME ZONE default NULL, Line 20: CONSTRAINT PK_qos_id PRIMARY KEY (id) 1. you can declare the colum as primary key 2. general question - are we gonna have here the columns for all "qos" hirerchy? if yes, what's the motivation for that and not to split it between relevant tables? 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 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 can be removed as it's the default 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 part. 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); wasn't it agreed to remove the indexes or to create one for both columns? http://gerrit.ovirt.org/#/c/27094/7/packaging/dbscripts/upgrade/pre_upgrade/0000_config.sql File packaging/dbscripts/upgrade/pre_upgrade/0000_config.sql: Line 221: select fn_db_add_config_value('NetworkQosSupported','false','3.2'); Line 222: select fn_db_add_config_value('StorageQosSupported','false','3.0'); Line 223: select fn_db_add_config_value('StorageQosSupported','false','3.1'); Line 224: select fn_db_add_config_value('StorageQosSupported','false','3.3'); Line 225: select fn_db_add_config_value('StorageQosSupported','false','3.4'); iirc there was some comment by eli in the last patchset. Line 226: select fn_db_add_config_value('HostNetworkQosSupported', 'false', '3.0'); Line 227: select fn_db_add_config_value('HostNetworkQosSupported', 'false', '3.1'); Line 228: select fn_db_add_config_value('HostNetworkQosSupported', 'false', '3.2'); Line 229: 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: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Kobi Ianko <k...@redhat.com> Gerrit-Reviewer: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
