Mike Kolesnik has posted comments on this change. Change subject: core: macPool per DC, db changes ......................................................................
Patch Set 33: Code-Review-1 (8 comments) http://gerrit.ovirt.org/#/c/26795/33/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/MacPool.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/MacPool.java: Line 83: this.description = description; Line 84: } Line 85: Line 86: @Override Line 87: public boolean equals(Object o) { Please have the equals & hashCode auto generated by the IDE to avoid common pitfalls. Line 88: if (this == o) { Line 89: return true; Line 90: } Line 91: if (!(o instanceof MacPool)) { http://gerrit.ovirt.org/#/c/26795/33/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/MacRange.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/MacRange.java: Line 39: this.macPoolId = macPoolId; Line 40: } Line 41: Line 42: @Override Line 43: public boolean equals(Object o) { Same about the equals and hashCode Line 44: if (this == o) { Line 45: return true; Line 46: } Line 47: if (!(o instanceof MacRange)) { http://gerrit.ovirt.org/#/c/26795/33/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/MacPoolDao.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/MacPoolDao.java: Line 8: public interface MacPoolDao extends GenericDao<MacPool, Guid> { Line 9: Line 10: MacPool getDefaultPool(); Line 11: Line 12: int getDCUsageCount(Guid id); Should be getDcUsageCount Line 13: Line 14: List<String> getAllMacsForMacPool(Guid macPoolId); Line 15: Line 16: MacPool getByDataCenterId(Guid id); http://gerrit.ovirt.org/#/c/26795/33/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 610: */ Line 611: public static final int NUMBER_OF_VMS_IN_VDS_GROUP_RHEL6_NFS_CLUSTER = 0; Line 612: public static final int NUMBER_OF_VMS_IN_VDS_GROUP_RHEL6_ISCSI = 7; Line 613: Line 614: public static final Guid NON_DEFAULT_MAC_POOL_ID2 = Guid.createGuidFromString("c248552d-64d4-4a77-ab1d-4c5eea00be6b"); 2 in the name is not needed since there's no NON_DEFAULT_MAC_POOL_ID Also, please use Guid constructor directly Line 615: public static final Guid NOT_USED_MAC_POOL_ID = Guid.createGuidFromString("17baf4e3-4347-4f81-915f-7762a108b81a"); Line 616: public static final Guid DEFAULT_MAC_POOL_ID = Guid.createGuidFromString("8c3a60e9-6a66-40db-84e0-c74077991775"); http://gerrit.ovirt.org/#/c/26795/33/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/MacPoolDaoTest.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/MacPoolDaoTest.java: Line 18: @Test Line 19: public void testGetDefaultPool() throws Exception { Line 20: final MacPool defaultPool = dao.getDefaultPool(); Line 21: assertThat(defaultPool, notNullValue()); Line 22: assertThat(defaultPool.getId(), is(FixturesTool.DEFAULT_MAC_POOL_ID)); I would additionally check that it's isDefault is true Line 23: } Line 24: Line 25: @Test Line 26: public void testGetDefaultMacPoolDcUsageCount() throws Exception { http://gerrit.ovirt.org/#/c/26795/33/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/utils/GuidUtils.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/utils/GuidUtils.java: Line 24: throw new MalformedIdException(e); Line 25: } Line 26: } Line 27: Line 28: public static Guid asGuidNullable(String id) { This isn't used so please don't add as part of this patch. Line 29: try { Line 30: return id == null ? null : new Guid(id); Line 31: } catch (IllegalArgumentException e) { Line 32: throw new MalformedIdException(e); http://gerrit.ovirt.org/#/c/26795/33/packaging/dbscripts/upgrade/03_05_0720_add_mac_pool_ranges_to_storage_pool.sql File packaging/dbscripts/upgrade/03_05_0720_add_mac_pool_ranges_to_storage_pool.sql: Line 7: ); Line 8: Line 9: CREATE TABLE mac_pool_ranges ( Line 10: mac_pool_id UUID NOT NULL REFERENCES mac_pools (id) ON DELETE CASCADE, Line 11: from_mac CHARACTER VARYING(17) NOT NULL, Ideally this should be simply CHARACTER(17) since you can't have other values here (longer or shorter), however I'm not sure if this would be supported by the DAL layer or not.. Line 12: to_mac CHARACTER VARYING(17) NOT NULL Line 13: ) WITH OIDS; Line 14: Line 15: SELECT fn_db_add_column('storage_pool', Line 34: INSERT INTO mac_pool_ranges (mac_pool_id, Line 35: from_mac, Line 36: to_mac) Line 37: SELECT (SELECT id FROM mac_pools), Line 38: mac_range[1], If the range start/end would contain a space then the insert would fail.. Probably good idea to trim it just to be safe. Line 39: mac_range[2] Line 40: FROM (SELECT string_to_array(unnest(string_to_array(option_value, ',')),'-') as mac_range Line 41: FROM vdc_options Line 42: WHERE option_name = 'MacPoolRanges') as mac_ranges; -- To view, visit http://gerrit.ovirt.org/26795 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id30f3c384ecf933daaacdbdd6542e88afb98f7ca Gerrit-PatchSet: 33 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@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
