Sergey Gotliv has posted comments on this change. Change subject: engine, dao: Introduce ISCSI Bond entity, tables and dao ......................................................................
Patch Set 7: (8 comments) http://gerrit.ovirt.org/#/c/22951/7/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 28: networkIds = new LinkedList<Guid>(); Line 29: } Line 30: Line 31: public IscsiBond(Guid id, Guid storagePoolId, String name, String description) { Line 32: networkIds = new LinkedList<Guid>(); > I'd just call this() instead. Done Line 33: this.id = id; Line 34: this.storagePoolId = storagePoolId; Line 35: this.name = name; Line 36: this.description = description; Line 57: @NotNull(message = "VALIDATION.ISCSI_BOND.NAME.NOT_NULL", groups = { CreateEntity.class, UpdateEntity.class }) Line 58: @Size(min = 1, max = 50, message = "VALIDATION.ISCSI_BOND.NAME.MAX", Line 59: groups = { CreateEntity.class, UpdateEntity.class }) Line 60: @ValidI18NName(message = "VALIDATION.ISCSI_BOND.NAME.INVALID.CHARACTER", Line 61: groups = { CreateEntity.class, UpdateEntity.class }) > shouldn't these be on the data members? JSR 303 spec allows to put it in on the member or on the getter. However I am moving it to the member to be consistent with the other parts of the code. Line 62: public String getName() { Line 63: return name; Line 64: } Line 65: Line 78: public void setDescription(String description) { Line 79: this.description = description; Line 80: } Line 81: Line 82: public List<Guid> getNetworkIds() { > Does GWT properly handle having a List here and not a concrete type? yes. Line 83: return networkIds; Line 84: } Line 85: Line 86: public void setNetworkIds(List<Guid> networkIds) { http://gerrit.ovirt.org/#/c/22951/7/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/VdcQueryType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/VdcQueryType.java: Line 224: GetStoragePoolsByClusterService(VdcQueryAuthType.User), Line 225: GetStorageDomainListById, Line 226: GetLunsByVgId, Line 227: GetPermittedStorageDomainsByStoragePoolId(VdcQueryAuthType.User), Line 228: GetIscsiBondsByStoragePoolId, > These two are strictly admin queries, right? For now - yes. Line 229: Line 230: // Event Notification Line 231: GetEventSubscribersBySubscriberIdGrouped, Line 232: http://gerrit.ovirt.org/#/c/22951/7/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 29: return getCallsHandler().executeReadList("GetNetworksByIscsiBondId", new RowMapper<Guid>() { Line 30: @Override Line 31: public Guid mapRow(ResultSet rs, int index) throws SQLException { Line 32: return new Guid((UUID) rs.getObject(1)); Line 33: } > just use createGuidMapper() Done Line 34: }, getCustomMapSqlParameterSource().addValue("iscsi_bond_id", iscsiBondId)); Line 35: } Line 36: Line 37: @Override Line 55: return getCallsHandler().executeReadList("GetConnectionsByIscsiBondId", new RowMapper<String>() { Line 56: @Override Line 57: public String mapRow(ResultSet rs, int index) throws SQLException { Line 58: return rs.getString(1); Line 59: } > I wonder if we should add a StringMapper next to IntegerMapper and BooleanM Sounds like a good idea. Line 60: }, getCustomMapSqlParameterSource().addValue("iscsi_bond_id", iscsiBondId)); Line 61: } Line 62: Line 63: @Override http://gerrit.ovirt.org/#/c/22951/7/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties: Line 1105: POWER_MANAGEMENT_ACTION_ON_ENTITY_ALREADY_IN_PROGRESS=Cannot perform ${action}. Another power management action is already in progress. Line 1106: LABELED_NETWORK_INTERFACE_NOT_FOUND=A labeled network interface could not be found. Line 1107: NETWORK_LABEL_CONFLICT=The networks represented by label cannot be configured on the same network interface. Line 1108: Line 1109: VALIDATION.ISCSI_BOND.NAME.MAX=Iscsi bond name must not exceed 50 characters > /s/Iscsi/iSCSI Done, for all comments in this file. Line 1110: VALIDATION.ISCSI_BOND.NAME.NOT_NULL=Iscsi bond name is required Line 1111: VALIDATION.ISCSI_BOND.DESCRIPTION.MAX=Iscsi bond description must not exceed 4000 characters Line 1112: VALIDATION.ISCSI_BOND.NAME.INVALID.CHARACTER=Iscsi Bond name must be formed from alpha-numeric characters or "-_." http://gerrit.ovirt.org/#/c/22951/7/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/IscsiBondDaoTest.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/IscsiBondDaoTest.java: Line 28: dao = dbFacade.getIscsiBondDao(); Line 29: storagePoolId = new Guid("6d849ebf-755f-4552-ad09-9a090cda105d"); Line 30: iscsiBondId = new Guid("7368f2be-1263-41f8-b95e-70cdaf23b80d"); Line 31: networkId = new Guid("58d5c1c6-cb15-4832-b2a4-023770607190"); Line 32: connectionId = "0cc146e8-e5ed-482c-8814-270bc48c297e"; > Please see my last comment. Done Line 33: Line 34: newIscsiBond = new IscsiBond(); Line 35: newIscsiBond.setId(Guid.newGuid()); Line 36: newIscsiBond.setName("Multipath"); -- 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: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Sergey Gotliv <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Sergey Gotliv <[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
