Martin Mucha has posted comments on this change. Change subject: engine: Add NetworkAttachment dao ......................................................................
Patch Set 19: (10 comments) https://gerrit.ovirt.org/#/c/32581/19/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/network/NetworkAttachmentDaoDbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/network/NetworkAttachmentDaoDbFacadeImpl.java: Line 47: SerializationFactory.getSerializer().serialize(networkAttachment.getProperties())); Line 48: Line 49: IpConfiguration ipConfiguration = networkAttachment.getIpConfiguration(); Line 50: if (ipConfiguration == null) { Line 51: mapper.addValue("boot_protocol", null) > Consider changing lines 50-60 to- extracted method, ternary for selecting what to map. Done. Line 52: .addValue("address", null) Line 53: .addValue("netmask", null) Line 54: .addValue("gateway", null); Line 55: } else { https://gerrit.ovirt.org/#/c/32581/19/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/network/NetworkAttachmentDaoTest.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/network/NetworkAttachmentDaoTest.java: Line 53: NetworkAttachment result = dao.get(FixturesTool.NETWORK_ATTACHMENT); Line 54: Line 55: assertNotNull(result); Line 56: assertEquals(FixturesTool.NETWORK_ATTACHMENT, result.getId()); Line 57: assertEquals(NetworkBootProtocol.DHCP, result.getIpConfiguration().getBootProtocol()); > Please check all the properties (network_id, nic_id, ip, gateway, netmask, I have no idea how do you want to achieve that. I've created method to create NetworkAttachment from fixtures, which contains dangerous duplicates. Is there a better way? Line 58: } Line 59: Line 60: /** Line 61: * Ensures that network attachments are returned. Line 98: dao.save(networkAttachment); Line 99: NetworkAttachment result = dao.get(networkAttachment.getId()); Line 100: assertNotNull(result); Line 101: assertEquals(networkAttachment.getId(), result.getId()); Line 102: assertEquals(NetworkBootProtocol.DHCP, result.getIpConfiguration().getBootProtocol()); > Same here, please check that all the fields are ok (you can add networkAtta Done Line 103: } Line 104: Line 105: /** Line 106: * Ensures that the update is working correctly Line 108: @Test Line 109: public void testUpdate() { Line 110: dao.save(networkAttachment); Line 111: networkAttachment.getIpConfiguration().setBootProtocol(NetworkBootProtocol.NONE); Line 112: Map<String, String> properties = new HashMap<>(); > Please update all the fields not just the properties one. Done Line 113: properties.put("key", "value"); Line 114: networkAttachment.setProperties(properties); Line 115: dao.update(networkAttachment); Line 116: NetworkAttachment result = dao.get(networkAttachment.getId()); Line 114: networkAttachment.setProperties(properties); Line 115: dao.update(networkAttachment); Line 116: NetworkAttachment result = dao.get(networkAttachment.getId()); Line 117: assertNotNull(result); Line 118: assertEquals(networkAttachment.getId(), result.getId()); > Same here- please use networkAttachmentEquals Done Line 119: assertEquals(NetworkBootProtocol.NONE, result.getIpConfiguration().getBootProtocol()); Line 120: assertEquals(properties, result.getProperties()); Line 121: } Line 122: https://gerrit.ovirt.org/#/c/32581/19/backend/manager/modules/dal/src/test/resources/fixtures.xml File backend/manager/modules/dal/src/test/resources/fixtures.xml: Line 5731: <column>_update_date</column> Line 5732: <row> Line 5733: <value>fd81f1f1-785b-4579-ab75-1419ebb87051</value> Line 5734: <value>58d5c1c6-cb15-4832-b2a4-023770607189</value> Line 5735: <value>ba31682e-6ae7-4f9d-8c6f-04c93acca9dd</value> > Please add attachments to different nics on the same host and to at least o Done Line 5736: <value>DHCP</value> Line 5737: <null /> Line 5738: <null /> Line 5739: <null /> https://gerrit.ovirt.org/#/c/32581/19/packaging/dbscripts/network_sp.sql File packaging/dbscripts/network_sp.sql: Line 1588: BEGIN Line 1589: Line 1590: RETURN QUERY SELECT * Line 1591: FROM network_attachments Line 1592: WHERE EXISTS (SELECT 1 > It means that the nic_id shouldn't be null (since there is no column for ho Done Line 1593: FROM vds_interface Line 1594: WHERE network_attachments.nic_id = vds_interface.id Line 1595: AND vds_interface.vds_id = v_host_id); Line 1596: https://gerrit.ovirt.org/#/c/32581/19/packaging/dbscripts/upgrade/03_06_1170_add_network_attachment.sql File packaging/dbscripts/upgrade/03_06_1170_add_network_attachment.sql: Line 4: CREATE TABLE network_attachments Line 5: ( Line 6: id UUID NOT NULL CONSTRAINT pk_network_attachments_id PRIMARY KEY, Line 7: network_id UUID NOT NULL, Line 8: nic_id UUID, > Please see previous comment- should be NOT NULL- GetNetworkAttachmentsByHos Done Line 9: boot_protocol CHARACTER VARYING(20), Line 10: address CHARACTER VARYING(20), Line 11: netmask CHARACTER VARYING(20), Line 12: gateway CHARACTER VARYING(20), Line 13: custom_properties TEXT, Line 14: _create_date TIMESTAMP WITH TIME ZONE DEFAULT ('now'::text)::timestamp without time zone, Line 15: _update_date TIMESTAMP WITH TIME ZONE, Line 16: FOREIGN KEY (network_id) REFERENCES network(id) ON DELETE CASCADE, Line 17: FOREIGN KEY (nic_id) REFERENCES vds_interface(id) ON DELETE SET NULL > please add unique constraint for (network_id, nic_id) Done Line 18: ); Line 19: Line 20: CREATE INDEX IDX_network_attachments_nic_id ON network_attachments(nic_id); Line 21: Line 13: custom_properties TEXT, Line 14: _create_date TIMESTAMP WITH TIME ZONE DEFAULT ('now'::text)::timestamp without time zone, Line 15: _update_date TIMESTAMP WITH TIME ZONE, Line 16: FOREIGN KEY (network_id) REFERENCES network(id) ON DELETE CASCADE, Line 17: FOREIGN KEY (nic_id) REFERENCES vds_interface(id) ON DELETE SET NULL > Since in this phase the nicless network is not supported and nic_id cannot I understand that 'set null' is wrong, when we've added 'not null' constraint. Question: is vds_interface removal user induced action or it's done for example as a part of some refresh action? If it's the latter, 'on delete cascade' makes sense, but if it's the former, usage of vds_interface by network_attachment record should be reported to the user and explicitely deleted. I think it's induced by refresh action, but I'm not sure. Line 18: ); Line 19: Line 20: CREATE INDEX IDX_network_attachments_nic_id ON network_attachments(nic_id); Line 21: -- To view, visit https://gerrit.ovirt.org/32581 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I84187f3900b9e3f8a917fc4e4126de9e50e231b4 Gerrit-PatchSet: 19 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Moti Asayag <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
