Yevgeny Zaspitsky has posted comments on this change. Change subject: backend: engine resolving host's active interface ......................................................................
Patch Set 7: (6 comments) https://gerrit.ovirt.org/#/c/35895/7/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/network/predicate/InterfaceByAddressPredicate.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/network/predicate/InterfaceByAddressPredicate.java: Line 9: @NotNull IMHO that do nothing as nothing validates that. You should use java.util.Objects.requireNonNull(managementAddress) within the constructor or alternatively return false upon eval method invocation in case of managementAddress==null. BTW, I'd prefer the last like you did in InterfaceByNetworkNamePredicate. https://gerrit.ovirt.org/#/c/35895/7/backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/network/predicate/VdsNetworkInterfaceByAddressPredicateTest.java File backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/network/predicate/VdsNetworkInterfaceByAddressPredicateTest.java: Line 2: Line 3: import org.junit.Before; Line 4: import org.ovirt.engine.core.common.businessentities.network.VdsNetworkInterface; Line 5: Line 6: public class VdsNetworkInterfaceByAddressPredicateTest extends AbstractVdsNetworkInterfacePredicateTest implements VdsNetworkInterfacePredicateTest { Please format , this line is too long. Line 7: Line 8: @Before Line 9: public void setup() { Line 10: setUnderTest(new InterfaceByAddressPredicate(getVALID())); https://gerrit.ovirt.org/#/c/35895/7/backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/network/predicate/VdsNetworkInterfaceByNetworkNamePredicateTest.java File backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/network/predicate/VdsNetworkInterfaceByNetworkNamePredicateTest.java: Line 2: Line 3: import org.junit.Before; Line 4: import org.ovirt.engine.core.common.businessentities.network.VdsNetworkInterface; Line 5: Line 6: public class VdsNetworkInterfaceByNetworkNamePredicateTest extends AbstractVdsNetworkInterfacePredicateTest implements VdsNetworkInterfacePredicateTest { Please format, this line is too long. Line 7: Line 8: @Before Line 9: public void setup() { Line 10: setUnderTest(new InterfaceByNetworkNamePredicate(getVALID())); https://gerrit.ovirt.org/#/c/35895/7/backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/network/predicate/VdsNetworkInterfacePredicateTest.java File backend/manager/modules/utils/src/test/java/org/ovirt/engine/core/utils/network/predicate/VdsNetworkInterfacePredicateTest.java: What the purpose of the test interface? Line 1: package org.ovirt.engine.core.utils.network.predicate; Line 2: Line 3: import org.ovirt.engine.core.common.businessentities.network.VdsNetworkInterface; Line 4: https://gerrit.ovirt.org/#/c/35895/7/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 1489: // Networks collection (name point to list of nics or bonds) : Map<String, Map<String, Object>> networks = : (Map<String, Map<String, Object>>) xmlRpcStruct.get(VdsProperties.NETWORKS); : Map<String, Map<String, Object>> bridges = : (Map<String, Map<String, Object>>) xmlRpcStruct.get(VdsProperties.NETWORK_BRIDGES); : Map<String, VdsNetworkInterface> vdsInterfaces = Entities.entitiesByName(vds.getInterfaces()); : boolean bridgesReported = FeatureSupported.bridgesReportByVdsm(vds.getVdsGroupCompatibilityVersion()); networks, vdsInterfaces and bridgesSupported do not belong here, please move this line lower, just before "if (networks != null)" line 1511 Line 1497: if (bridges != null) { : for (Entry<String, Map<String, Object>> entry : bridges.entrySet()) { : Map<String, Object> bridgeProperties = entry.getValue(); : String bridgeName = entry.getKey(); : if (bridgeProperties != null) { : String bridgeAddress = (String) bridgeProperties.get("addr"); : //in case host is communicating with engine over a bridge : if (bridgeAddress != null && bridgeAddress.equals(hostIp)) { : hostActiveNic = bridgeName; : } : } : } : } please extract that into a method with some meaningful name -- 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: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eliraz Levi <el...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Eliraz Levi <el...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
