Martin Mucha has posted comments on this change. Change subject: engine: Extract common interface logic to single method ......................................................................
Patch Set 1: Code-Review-1 (5 comments) I tried really hard to verify if any line isn't missing after refactoring or isn't executed extra. I did not find any, but I'm not super-sure about it. http://gerrit.ovirt.org/#/c/37078/1/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsBrokerObjectsBuilder.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsBrokerObjectsBuilder.java: Line 1413: VdsNetworkInterface iface = new Bond(); Line 1414: updateCommonInterfaceData(iface, vds, entry); Line 1415: iface.setBonded(true); Line 1416: Line 1417: Map<String, Object> bond = (Map<String, Object>) entry.getValue(); I know this is not your code, but maybe we can fix it in this patch(or following), but same problem is in newly created method below. maybe we could rename this. I have little understanding of this, but 'bond' seems to be placed in 'iface' variable, which I think should be named bond. This is (probably) bondParameters? Line 1418: if (bond != null) { Line 1419: iface.setMacAddress((String) bond.get("hwaddr")); Line 1420: if (bond.get("slaves") != null) { Line 1421: addBondDeviceToHost(vds, iface, (Object[]) bond.get("slaves")); Line 1479: for (Entry<String, Object> entry : nics.entrySet()) { Line 1480: VdsNetworkInterface iface = new Nic(); Line 1481: updateCommonInterfaceData(iface, vds, entry); Line 1482: Line 1483: Map<String, Object> nic = (Map<String, Object>) entry.getValue(); improper name Line 1484: if (nic != null) { Line 1485: if (nic.get("speed") != null) { Line 1486: Object speed = nic.get("speed"); Line 1487: iface.setSpeed((Integer) speed); Line 1507: * @param iface Line 1508: * The interface to update Line 1509: * @param vds Line 1510: * The host to which the interface belongs. Line 1511: * @param nic inexisting parameter Line 1512: * A key-value map of the interface properties and their value Line 1513: */ Line 1514: private static void updateCommonInterfaceData(VdsNetworkInterface iface, VDS vds, Entry<String, Object> entry) { Line 1515: iface.setName(entry.getKey()); Line 1510: * The host to which the interface belongs. Line 1511: * @param nic Line 1512: * A key-value map of the interface properties and their value Line 1513: */ Line 1514: private static void updateCommonInterfaceData(VdsNetworkInterface iface, VDS vds, Entry<String, Object> entry) { I'd rather avoid using Entry class, since it has no meaning, it's just pair, and this method is not readable without consulting caller method and figuring out, what pair is that. above said especially holds, when passing common types like String and Object, and in that Object, there's stored Map to which we're unsafely casting. Line 1515: iface.setName(entry.getKey()); Line 1516: iface.setId(Guid.newGuid()); Line 1517: iface.setVdsId(vds.getId()); Line 1518: Line 1520: iStats.setId(iface.getId()); Line 1521: iStats.setVdsId(vds.getId()); Line 1522: iface.setStatistics(iStats); Line 1523: Line 1524: Map<String, Object> nic = (Map<String, Object>) entry.getValue(); improper name. Line 1525: if (nic != null) { Line 1526: iface.setAddress((String) nic.get("addr")); Line 1527: iface.setSubnet((String) nic.get("netmask")); Line 1528: -- To view, visit http://gerrit.ovirt.org/37078 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I495f36cea0a63a161ca4ae85fc08e67e718befa2 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Lior Vernia <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Martin Mucha <[email protected]> Gerrit-Reviewer: [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
