Alona Kaplan has posted comments on this change. Change subject: engine: Add NetworkAttachment dao ......................................................................
Patch Set 19: (9 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 42: protected MapSqlParameterSource createFullParametersMapper(NetworkAttachment networkAttachment) { Line 43: MapSqlParameterSource mapper = createIdParameterMapper(networkAttachment.getId()) Line 44: .addValue("network_id", networkAttachment.getNetworkId()) Line 45: .addValue("nic_id", networkAttachment.getNicId()) Line 46: .addValue("custom_properties", Please add a separate table for custom properties (table columns- network_attachment_id, key, value) instead of storing the values as a blob. Line 47: SerializationFactory.getSerializer().serialize(networkAttachment.getProperties())); Line 48: Line 49: IpConfiguration ipConfiguration = networkAttachment.getIpConfiguration(); Line 50: if (ipConfiguration == null) { 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- if (ipConfiguration == null) { ipConfiguration = new IpConfiguration(); } mapper.addValue("boot_protocol", EnumUtils.nameOrNull(ipConfiguration.getBootProtocol())) .addValue("address", ipConfiguration.getAddress()) .addValue("netmask", ipConfiguration.getNetmask()) .addValue("gateway", ipConfiguration.getGateway()); 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, custom_properties). 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 networkAttachmentEquals method for this purpose). 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. 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 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 one more host so that the tests will be more interesting:) 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 host_id, nic_id is the only way to find the host to which the attachment belongs). Please update the NetworkAttahcment entity with notNull annotation on the field and mark the table column as NOT NULL. 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- GetNetworkAttachmentsByHostId resides on it. 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), -- 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
