Yevgeny Zaspitsky has posted comments on this change. Change subject: backend: engine resolving host's active interface ......................................................................
Patch Set 6: (9 comments) Pls add unit-tests. https://gerrit.ovirt.org/#/c/35895/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/NetworkConfigurator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/NetworkConfigurator.java: Line 163: : : : : : Why did you removed this check? Line 33: : import java.util.ArrayList; : import java.util.HashMap; : import java.util.List; : import java.util.Map; : import java.util.concurrent.TimeUnit; : please put java.* imports first and setup your IDE settings for that Line 64: : log.info("The management network '{}' is already configured on host '{}'", : managementNetworkName, : host.getName()); the message does not match the check. pls separate checking mmanagement network name from the IP check. Line 100: : private String resolveHostManagementNetworkAddress(String hostManagmentNetworkName){ : for (VdsNetworkInterface iface : host.getInterfaces()){ : if (iface.getNetworkName() != null && iface.getNetworkName().equals(hostManagmentNetworkName)) { : return iface.getAddress(); : } : } : : return null; : } this looks like findNicByNetworkName functionality. Iguess we already have that piece of code... https://gerrit.ovirt.org/#/c/35895/6/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/NetworkUtils.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/NetworkUtils.java: Line 289: Logger log Why do that need callee's logger? The local one could be used instead. Line 290: // TODO elevi check in case of null in case of null loopback address will be returned. Is that what we want? Line 290: : String output = null; : InetAddress address = null; : try { : address = InetAddress.getByName(vds.getHostName()); : output = address.getHostAddress().trim(); : } catch (Exception ex) { : log.warn("Fail to resolve host ip by name '{}', Details: '{}' ", vds.getHostName(), ex.toString()); : log.debug("Exception", ex); : } finally { : return output; : } I'd write it that way: try { final InetAddress address = InetAddress.getByName(vds.getHostName()); return address.getHostAddress().trim(); } catch (UnknownHostException e) { final String msg = "Fail to resolve host ip by name '{}'"; log.warn(msg + ", Details: '{}' ", vds.getHostName(), e.getCause()); log.debug(msg, vds.getHostName(), e); return null; } https://gerrit.ovirt.org/#/c/35895/6/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 1469: resolveActiveNic the mmethod name does not describe wjat method does Line 1505: setActiveNic here you might be overriding the value that you already set few instructions earlier. I'd prefer that fer every scenario setActiveNic would be called only once, meaning fisrst evaluating all conditionss and finding the right value then calling set method. IMHO that's more structured approach. -- To view, visit https://gerrit.ovirt.org/35895 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I64a21595e58358fe9e04ea1de5d3e3115c7bc2c6 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eliraz Levi <[email protected]> Gerrit-Reviewer: Alona Kaplan <[email protected]> Gerrit-Reviewer: Eliraz Levi <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Yevgeny Zaspitsky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
