Eliraz Levi 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 exac Done 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?... Done 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, s Done 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 e Done 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 b Done 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: Done 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. Done 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... Done 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 cor Done 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. Done 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 getNetworkI Done 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. Done 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 attache Done 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 <[email protected]> Gerrit-Reviewer: Eliraz Levi <[email protected]> Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: Moti Asayag <[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
