Martin Mucha has posted comments on this change. Change subject: core: macPool per DC, db changes ......................................................................
Patch Set 33: (8 comments) answers. 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 a) I didn't know for which fields we want to check identity, so I just add mine changes. What's the usual approach — check everything? I do believe you said this, just checking. b) now I deleted original implementation and regenerated those two methods from scratch for all fields. Result is exactly same as in previously posted patch. So DONE. c) in which way you define hashcode equals? — for example there's some distinction between rules for db entities when using hibernate vs eclipselink, but we're using our own approach. Can be instance of some class equal to it's parent class instance? Should implementation of hashcode/equals allow that or prohibit? 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 same, DONE. 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 Done 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 Done 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 Done 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. Done 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 valu dal should handle this, this is older db type. there probably will not be any difference, we just save 8 letters by this. DONE. 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.. P I'm against this. No clever fixing of errors. This is creation of gotchas. Like automatic type inference we've encountered together. This is update script, here this strict rule could be ignored, but not aware of any such corrections in former solution, so there's no reason for introducing them now. 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
