Moti Asayag has posted comments on this change. Change subject: db: aggregate qos and storage qos impl ......................................................................
Patch Set 6: (4 comments) 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 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); > When we'll get to the bridge, we'll cross it. sorry. i disagree on this approach and can't see the smartness in this solution. We don't have any LOC criteria we should follow and we don't measure by how short is our code. However good design is a must. With this design there is a violation of separation of concerns. You let the DAO to act as a mapper. This is not clean. I'd like to hear Allon's comment on this issue - for second opinion. Line 36: Line 37: /** Line 38: * @return qos type for derived qos dao Line 39: */ Line 85: return getCustomMapSqlParameterSource() Line 86: .addValue("id", guid); Line 87: } Line 88: Line 89: protected Integer getIntegerOrNull(ResultSet rs, String columnName) throws SQLException { > will do it in vnic patch. don't want to fix it twice. Why twice ? Just add the method as above to BaseDAODbFacade and use it instead of the function introduced here. Later-on, send a patch for whatever should be fix/adjusted. Line 90: int i = rs.getInt(columnName); Line 91: return rs.wasNull() ? null : i; Line 92: } 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, > good to know, btw I'm the component owner of sla and scheduling. Sorry for not being precise on my quote, see: https://www.mail-archive.com/[email protected]/msg00492.html and specifically from Barak's comment: "Once this is done - each maintainer/reviewer should start enforcing that policy in his reviews." And here i'm enforcing that convention. There is less sense in sending a lot of upgrade script for renaming the tables, stored procedure, views, business entities, commands, queries and ... 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; > currently validation. Could you explain what do you mean by 'validation' ? 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
