Liron Ar has posted comments on this change. Change subject: engine, dao: Introduce ISCSI Bond entity, tables and dao ......................................................................
Patch Set 11: (9 comments) http://gerrit.ovirt.org/#/c/22951/11/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/GetStorageServerConnectionsByStoragePoolIdQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/GetStorageServerConnectionsByStoragePoolIdQuery.java: Line 2: Line 3: import org.ovirt.engine.core.bll.QueriesCommandBase; Line 4: import org.ovirt.engine.core.common.queries.IdQueryParameters; Line 5: Line 6: public class GetStorageServerConnectionsByStoragePoolIdQuery<P extends IdQueryParameters> extends QueriesCommandBase<P> { IMO the query name should be changed to reflect that only "connectable" would be returned. Line 7: Line 8: public GetStorageServerConnectionsByStoragePoolIdQuery(P parameters) { Line 9: super(parameters); Line 10: } http://gerrit.ovirt.org/#/c/22951/11/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/GetIscsiBondsByStoragePoolIdQueryTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/GetIscsiBondsByStoragePoolIdQueryTest.java: Line 29: Line 30: when(getQueryParameters().getId()).thenReturn(storagePoolId); Line 31: when(getDbFacadeMockInstance().getIscsiBondDao()).thenReturn(iscsiBondDao); Line 32: when(iscsiBondDao.getAllByStoragePoolId(storagePoolId)).thenReturn(Collections.singletonList(iscsiBond)); Line 33: when(iscsiBondDao.getNetworkIdsByIscsiBondId((Guid) anyObject())).thenReturn(Collections.singletonList(networkId)); why? we already have the ids - it's better to have this mocked with the id's that we created, that way the test will check that all the calls are with the correct ids and will avoid breaking of the query./ Line 34: when(iscsiBondDao.getStorageConnectionIdsByIscsiBondId((Guid) anyObject())).thenReturn(Collections.singletonList(connectionId)); Line 35: Line 36: getQuery().executeQueryCommand(); Line 37: Line 40: assertEquals(iscsiBond, result.get(0)); Line 41: assertEquals(1, iscsiBond.getNetworkIds().size()); Line 42: assertEquals(iscsiBond.getNetworkIds().get(0), networkId); Line 43: assertEquals(1, iscsiBond.getStorageConnectionIds().size()); Line 44: assertEquals(iscsiBond.getStorageConnectionIds().get(0), connectionId); perhaps better to add assertions to the sizes before accessing the lists, to avoid NPE and have clearer message when it's broken. Line 45: } http://gerrit.ovirt.org/#/c/22951/11/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/IscsiBond.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/IscsiBond.java: Line 32: @ValidDescription(message = "VALIDATION_ISCSI_BOND_DESCRIPTION_INVALID", Line 33: groups = { CreateEntity.class, UpdateEntity.class }) Line 34: private String description; Line 35: private List<Guid> networkIds; Line 36: private List<String> storageConnectionIds; please have all the members defined together... Line 37: Line 38: public IscsiBond() { Line 39: networkIds = new LinkedList<Guid>(); Line 40: } Line 64: public void setStoragePoolId(Guid storagePoolId) { Line 65: this.storagePoolId = storagePoolId; Line 66: } Line 67: Line 68: public String getName() { we have some interface for objects with name, please check if you can add this as implementation of it. Line 69: return name; Line 70: } Line 71: Line 72: public void setName(String name) { Line 72: public void setName(String name) { Line 73: this.name = name; Line 74: } Line 75: Line 76: public String getDescription() { same, i think that we might have interface for that..so you can simple add it to the class definition. Line 77: return description; Line 78: } Line 79: Line 80: public void setDescription(String description) { http://gerrit.ovirt.org/#/c/22951/11/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/IscsiBondDao.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/IscsiBondDao.java: Line 16: public void removeNetworkFromIscsiBond(Guid iscsiBondId, Guid networkId); Line 17: Line 18: public List<String> getStorageConnectionIdsByIscsiBondId(Guid iscsiBondId); Line 19: Line 20: public void addStorageConnectionToIscsiBond(Guid iscsiBondId, String storageConnectionId); extra space here Line 21: Line 22: public void removeStorageConnectionFromIscsiBond(Guid iscsiBondId, String storageConnectionId); http://gerrit.ovirt.org/#/c/22951/11/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/IscsiBondDaoDbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/IscsiBondDaoDbFacadeImpl.java: Line 91: @Override Line 92: public IscsiBond mapRow(ResultSet rs, int rowNum) throws SQLException { Line 93: IscsiBond entity = new IscsiBond(); Line 94: Line 95: entity.setId(getGuidDefaultNewGuid(rs, "id")); if we don't have id returned for some reason we don't want a default new guid - it's error prone. Line 96: entity.setDescription(rs.getString("description")); Line 97: entity.setName(rs.getString("name")); Line 98: entity.setStoragePoolId(getGuidDefaultNewGuid(rs, "storage_pool_id")); Line 99: return entity; http://gerrit.ovirt.org/#/c/22951/11/packaging/dbscripts/upgrade/03_04_0500_add_iscsi_bond.sql File packaging/dbscripts/upgrade/03_04_0500_add_iscsi_bond.sql: Line 25: CREATE TABLE iscsi_bonds_storage_connections_map Line 26: ( Line 27: iscsi_bond_id UUID NOT NULL, Line 28: connection_id varchar(50) NOT NULL, Line 29: CONSTRAINT PK_iscsi_bonds_storage_connections_map PRIMARY KEY(iscsi_bond_id,connection_id), can connection be part of few iscsi bonds? if not, the primary key here is too weak. Line 30: CONSTRAINT FK_iscsi_bonds_storage_connections_map_iscsi_bond_id FOREIGN KEY(iscsi_bond_id) Line 31: REFERENCES iscsi_bonds(id) ON DELETE CASCADE, Line 32: CONSTRAINT FK_iscsi_bonds_storage_connections_map_connection_id FOREIGN KEY(connection_id) Line 33: REFERENCES storage_server_connections(id) ON DELETE CASCADE -- To view, visit http://gerrit.ovirt.org/22951 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I12313c02810a2f0e75016bdd78b44da43f2154d4 Gerrit-PatchSet: 11 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Sergey Gotliv <sgot...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Amador Pahim <apa...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Sergey Gotliv <sgot...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com> 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
