Lior Vernia has posted comments on this change. Change subject: webadmin: adding sync column to subtab net host ......................................................................
Patch Set 3: (13 comments) http://gerrit.ovirt.org/#/c/35697/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/GetVdsAndNetworkInterfacesByNetworkIdQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/network/host/GetVdsAndNetworkInterfacesByNetworkIdQuery.java: Line 36: Line 37: final Map<Guid, VDS> vdsById = Entities.businessEntitiesById(vdsList); Line 38: Line 39: HostNetworkQosDao qosDao = getDbFacade().getHostNetworkQosDao(); Line 40: Map<String, Network> networks = Entities.entitiesByName( Why do you need to retrieve all the networks in a DC? You already know exactly which network you're interested in, you may just call getNetworksDao().get(getParameters().getId()). Line 41: getDbFacade().getNetworkDao().getAll()); Line 42: Line 43: List<PairQueryable<VdsNetworkInterface, VDS>> vdsInterfaceVdsPairs = Line 44: new ArrayList<PairQueryable<VdsNetworkInterface, VDS>>(); Line 45: for (final VdsNetworkInterface vdsNetworkInterface : vdsNetworkInterfaceList) { Line 46: vdsInterfaceVdsPairs.add(new PairQueryable<VdsNetworkInterface, VDS>(vdsNetworkInterface, Line 47: vdsById.get(vdsNetworkInterface.getVdsId()))); Line 48: Line 49: if (!Boolean.TRUE.equals(vdsNetworkInterface.getBonded()) Why do you care if an interface is a bond or not?... Line 50: || LinqUtils.filter(vdsNetworkInterfaceList, new Predicate<VdsNetworkInterface>() { Line 51: @Override public boolean eval(VdsNetworkInterface bond) { Line 52: return StringUtils.equals(bond.getBondName(), vdsNetworkInterface.getName()); Line 53: } Line 46: vdsInterfaceVdsPairs.add(new PairQueryable<VdsNetworkInterface, VDS>(vdsNetworkInterface, Line 47: vdsById.get(vdsNetworkInterface.getVdsId()))); Line 48: Line 49: if (!Boolean.TRUE.equals(vdsNetworkInterface.getBonded()) Line 50: || LinqUtils.filter(vdsNetworkInterfaceList, new Predicate<VdsNetworkInterface>() { Why do you care to find whether a bond has any interfaces?... Either way, shouldn't be possible due to our backend logic. Line 51: @Override public boolean eval(VdsNetworkInterface bond) { Line 52: return StringUtils.equals(bond.getBondName(), vdsNetworkInterface.getName()); Line 53: } Line 54: }).size() > 0) { Line 52: return StringUtils.equals(bond.getBondName(), vdsNetworkInterface.getName()); Line 53: } Line 54: }).size() > 0) { Line 55: Network network = networks.get(vdsNetworkInterface.getNetworkName()); Line 56: vdsNetworkInterface.setNetworkImplementationDetails(NetworkUtils.calculateNetworkImplementationDetails(network, I think this should be called outside of any condition clause, simply for every interface in vdsNetworkInterfaceList. Line 57: network == null ? null : qosDao.get(network.getQosId()), Line 58: vdsNetworkInterface)); Line 59: } Line 60: Line 53: } Line 54: }).size() > 0) { Line 55: Network network = networks.get(vdsNetworkInterface.getNetworkName()); Line 56: vdsNetworkInterface.setNetworkImplementationDetails(NetworkUtils.calculateNetworkImplementationDetails(network, Line 57: network == null ? null : qosDao.get(network.getQosId()), How could network be null?... This is a query used to retrieve interfaces by network ID, it's unlikely to be used for a null network. Line 58: vdsNetworkInterface)); Line 59: } Line 60: Line 61: http://gerrit.ovirt.org/#/c/35697/3/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/host/GetVdsAndNetworkInterfacesByNetworkIdQueryTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/network/host/GetVdsAndNetworkInterfacesByNetworkIdQueryTest.java: Line 61: List<VdsNetworkInterface> expectedVdsNetworkInterface = Collections.singletonList(vdsNetworkInterface); Line 62: InterfaceDao vdsNetworkInterfaceDaoMock = mock(InterfaceDao.class); Line 63: when(vdsNetworkInterfaceDaoMock.getVdsInterfacesByNetworkId(networkId)).thenReturn(expectedVdsNetworkInterface); Line 64: when(getDbFacadeMockInstance().getInterfaceDao()).thenReturn(vdsNetworkInterfaceDaoMock); Line 65: NetworkDao networkDaoMock = mock(NetworkDao.class); I would: 1. Put this in a separate method. 2. Create a mock network to be returned by the networkDao - otherwise it doesn't reflect proper behavior... It'll return null for a network, which shouldn't really happen in real-life scenarios. 3. Also mock qosDao (in a separate method). The out-of-sync functionality should already be tested in NetworkUtilsTest, so not necessary to test it here. Line 66: when(getDbFacadeMockInstance().getNetworkDao()).thenReturn(networkDaoMock); Line 67: } http://gerrit.ovirt.org/#/c/35697/3/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/ApplicationConstants.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/ApplicationConstants.java: Line 1277: Line 1278: @DefaultStringValue("Data Center") Line 1279: String networkPopupDataCenterLabel(); Line 1280: Line 1281: @DefaultStringValue("Out Of Sync") "Out-of-sync" would be more consistent with other instances. Line 1282: String hostOutOfSync(); Line 1283: Line 1284: // Quota Storage Line 1285: @DefaultStringValue("Name") http://gerrit.ovirt.org/#/c/35697/3/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/network/SubTabNetworkHostView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/network/SubTabNetworkHostView.java: Line 91: getTable().setTableTopMargin(20); Line 92: } Line 93: Line 94: private final HostStatusColumn<PairQueryable<VdsNetworkInterface, VDS>> hostStatus = new HostStatusColumn<PairQueryable<VdsNetworkInterface, VDS>>(); Line 95: One blank line is usually enough to separate code blocks... Line 96: Line 97: WebAdminImageResourceColumn<PairQueryable<VdsNetworkInterface, VDS>> hostOutOfSync = new WebAdminImageResourceColumn<PairQueryable<VdsNetworkInterface, VDS>>(){ Line 98: Line 99: @Override Line 93: Line 94: private final HostStatusColumn<PairQueryable<VdsNetworkInterface, VDS>> hostStatus = new HostStatusColumn<PairQueryable<VdsNetworkInterface, VDS>>(); Line 95: Line 96: Line 97: WebAdminImageResourceColumn<PairQueryable<VdsNetworkInterface, VDS>> hostOutOfSync = new WebAdminImageResourceColumn<PairQueryable<VdsNetworkInterface, VDS>>(){ How about making this member private and final? And declaring it in its correct position as it's displayed in the dialog, i.e. following nicStatusColumn? Line 98: Line 99: @Override Line 100: public ImageResource getValue(PairQueryable<VdsNetworkInterface, VDS> object) { Line 101: Line 97: WebAdminImageResourceColumn<PairQueryable<VdsNetworkInterface, VDS>> hostOutOfSync = new WebAdminImageResourceColumn<PairQueryable<VdsNetworkInterface, VDS>>(){ Line 98: Line 99: @Override Line 100: public ImageResource getValue(PairQueryable<VdsNetworkInterface, VDS> object) { Line 101: Again, a little spacey for my taste. Line 102: if (object.getFirst() != null && object.getFirst().getNetworkImplementationDetails() != null ){ Line 103: if (!object.getFirst().getNetworkImplementationDetails().isInSync()){ Line 104: return resources.networkNotSyncImage(); Line 105: } Line 98: Line 99: @Override Line 100: public ImageResource getValue(PairQueryable<VdsNetworkInterface, VDS> object) { Line 101: Line 102: if (object.getFirst() != null && object.getFirst().getNetworkImplementationDetails() != null ){ If object.getFirst() != null, then it shouldn't be possible for getNetworkImplementationDetails() to be null (based on your implementation). How about instead of all of this, simply: return (object.getFirst() == null || object.getFirst().getNetworkImplementationDetails().isInSync()) ? null : resources.networkNotSyncImage(); Line 103: if (!object.getFirst().getNetworkImplementationDetails().isInSync()){ Line 104: return resources.networkNotSyncImage(); Line 105: } Line 106: Line 109: return null; Line 110: } Line 111: Line 112: }; Line 113: Same comment as above about blank lines. Line 114: Line 115: private final TextColumnWithTooltip<PairQueryable<VdsNetworkInterface, VDS>> nameColumn = new TextColumnWithTooltip<PairQueryable<VdsNetworkInterface, VDS>>() { Line 116: @Override Line 117: public String getValue(PairQueryable<VdsNetworkInterface, VDS> object) { Line 225: getTable().ensureColumnPresent(nameColumn, constants.nameHost(), true, "200px"); //$NON-NLS-1$ Line 226: getTable().ensureColumnPresent(clusterColumn, constants.clusterHost(), true, "200px"); //$NON-NLS-1$ Line 227: getTable().ensureColumnPresent(dcColumn, constants.dcHost(), true, "200px"); //$NON-NLS-1$ Line 228: getTable().ensureColumnPresent(nicStatusColumn, constants.statusNetworkHost(), attached, "140px"); //$NON-NLS-1$ Line 229: getTable().ensureColumnPresent(hostOutOfSync, constants.hostOutOfSync(), true, "100px"); //$NON-NLS-1$ Why true? This is meaningless for a host to which the network isn't attached. Line 230: getTable().ensureColumnPresent(nicColumn, constants.nicNetworkHost(), attached, "100px"); //$NON-NLS-1$ Line 231: getTable().ensureColumnPresent(speedColumn, Line 232: templates.sub(constants.speedNetworkHost(), constants.mbps()).asString(), Line 233: attached, -- To view, visit http://gerrit.ovirt.org/35697 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4dd5d3a1e799d8ec0c8f8cea2d4ff7257d0234c2 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eliraz Levi <el...@redhat.com> Gerrit-Reviewer: Eliraz Levi <el...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
