Alexander Wels has posted comments on this change. Change subject: frontend: NUMA support ......................................................................
Patch Set 1: (15 comments) http://gerrit.ovirt.org/#/c/28299/1/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/HostListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/HostListModel.java: Line 2023: Line 2024: boolean numaEnabled = items.isEmpty() ? false : true; Line 2025: for (VDS vds : items) { Line 2026: if (!vds.isNumaSupport()) { Line 2027: numaEnabled = false; So NUMA is disabled if ANY of the hosts doesn't have NUMA support? Should this be any of the selected hosts doesn't have NUMA support? Line 2028: break; Line 2029: } Line 2030: } Line 2031: // getNumaSupportCommand().setIsExecutionAllowed(numaEnabled); http://gerrit.ovirt.org/#/c/28299/1/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/numa/NumaSupportModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/numa/NumaSupportModel.java: Line 36: private List<VdsNumaNode> numaNodeList; Line 37: private List<VM> vmsWithvNumaNodeList; Line 38: private List<VNodeModel> UnassignedVNodeModelList; Line 39: private List<VNodeModel> assignedVNodeModelList; Line 40: private Map<Guid, List<VNodeModel>> p2vNumaNodesMap; Make this final and initialize it here, then in the init function just clear it. This guarantees that the map is never null. Line 41: private List<Pair<Integer, Set<VdsNumaNode>>> firstLevelDistanceSetList; Line 42: private final Event modelReady = new Event(new EventDefinition("ModelReady", NumaSupportModel.class)); //$NON-NLS-1$ Line 43: Line 44: private Map<Integer, VdsNumaNode> indexNodeMap; Line 37: private List<VM> vmsWithvNumaNodeList; Line 38: private List<VNodeModel> UnassignedVNodeModelList; Line 39: private List<VNodeModel> assignedVNodeModelList; Line 40: private Map<Guid, List<VNodeModel>> p2vNumaNodesMap; Line 41: private List<Pair<Integer, Set<VdsNumaNode>>> firstLevelDistanceSetList; Make this final and initialize it here, then in the init function just clear it. This guarantees that the list is never null. Line 42: private final Event modelReady = new Event(new EventDefinition("ModelReady", NumaSupportModel.class)); //$NON-NLS-1$ Line 43: Line 44: private Map<Integer, VdsNumaNode> indexNodeMap; Line 45: Line 40: private Map<Guid, List<VNodeModel>> p2vNumaNodesMap; Line 41: private List<Pair<Integer, Set<VdsNumaNode>>> firstLevelDistanceSetList; Line 42: private final Event modelReady = new Event(new EventDefinition("ModelReady", NumaSupportModel.class)); //$NON-NLS-1$ Line 43: Line 44: private Map<Integer, VdsNumaNode> indexNodeMap; make this final and initialize it here or in constructor? Line 45: Line 46: public NumaSupportModel(List<VDS> hosts, VDS host, ListWithDetailsModel listModel) { Line 47: this.listModel = listModel; Line 48: setHosts(new ListModel<VDS>()); Line 56: } Line 57: Line 58: private void initHosts(List<VDS> hosts, VDS host) { Line 59: getHosts().setItems(hosts); Line 60: if (host == null) { Maybe consider checking if hosts contains host, so when we call setSelectedItem we know for sure that we are passing in a host that is part of the list so something like: if (host == null || !hosts.contains(host)) { host = hosts.get(0); } Line 61: host = hosts.get(0); Line 62: } Line 63: if (getHosts().getItems().size() <= 1) { Line 64: getHosts().setIsChangable(false); Line 88: @SuppressWarnings("unchecked") Line 89: @Override Line 90: public void onSuccess(Object model, Object returnValue) { Line 91: // TODO: host query can be skipped in case it was already fetched. Line 92: NumaSupportModel.this.getNumaNodeList().addAll((List<VdsNumaNode>) returnValue); Are we guaranteed to always get a List<VdsNumaNode>? If not might want to check the type of the object. Thinking about this some more, why is INewAsyncCallback not generic.. Not an issue in this patch, but something to put on my todo list. Line 93: NumaSupportModel.this.initFirstLevelDistanceSetList(); Line 94: AsyncDataProvider.getVMsWithVNumaNodesByClusterId(new AsyncQuery(new INewAsyncCallback() { Line 95: Line 96: @Override Line 95: Line 96: @Override Line 97: public void onSuccess(Object model, Object returnValue) { Line 98: NumaSupportModel.this.setVmsWithvNumaNodeList((List<VM>) returnValue); Line 99: NumaSupportModel.this.initVNumaNodes(); Can you rename this method to initVirtualNumaNodes(), I had to read it 3 times before I realized it didn't read initVMumaNodes(). Line 100: NumaSupportModel.this.modelReady(); Line 101: } Line 102: Line 103: }), NumaSupportModel.this.hosts.getSelectedItem().getVdsGroupId()); Line 108: Line 109: private void initVNumaNodes() { Line 110: assignedVNodeModelList = new ArrayList<VNodeModel>(); Line 111: UnassignedVNodeModelList = new ArrayList<VNodeModel>(); Line 112: p2vNumaNodesMap = new HashMap<Guid, List<VNodeModel>>(); if you make it final and init it earlier, you can just call clear here. Line 113: Line 114: for (VM vm : getVmsWithvNumaNodeList()) { Line 115: if (vm.getvNumaNodeList() != null) { Line 116: for (VmNumaNode vmNumaNode : vm.getvNumaNodeList()) { Line 138: UnassignedVNodeModelList.add(vNodeModel); Line 139: } Line 140: } Line 141: } Line 142: } Maybe have an else that throws an exception if the value is null. Or if null is an acceptable value do something like: List<VmNumaNode> virtualNumaNodeList = vm.getvNumaNodeList() == null ? Collections.EMPTY_LIST : vm.getvNumaNodeList(); for (VmNumaNode vmNumaNode : virtualNumaNodeList ) { ... } This saves an extra if statement and since we are nested pretty far this might be useful. Line 143: } Line 144: } Line 145: Line 146: private VdsNumaNode getNodeByIndex(Integer index) { Line 143: } Line 144: } Line 145: Line 146: private VdsNumaNode getNodeByIndex(Integer index) { Line 147: if (indexNodeMap == null) { if it is final and initialized in construct, no need to check here. Line 148: indexNodeMap = new HashMap<Integer, VdsNumaNode>(); Line 149: for (VdsNumaNode node : getNumaNodeList()) { Line 150: indexNodeMap.put(node.getIndex(), node); Line 151: } Line 153: return indexNodeMap.get(index); Line 154: } Line 155: Line 156: public List<VNodeModel> getVNumaNodeByNodeId(Guid nodeId) { Line 157: return p2vNumaNodesMap.get(nodeId); Unless you make it final and init it in constructor/declaration, there is no guarantee that this is not null when you call this. Line 158: } Line 159: Line 160: private void initFirstLevelDistanceSetList() { Line 161: firstLevelDistanceSetList = new ArrayList<Pair<Integer, Set<VdsNumaNode>>>(); Line 157: return p2vNumaNodesMap.get(nodeId); Line 158: } Line 159: Line 160: private void initFirstLevelDistanceSetList() { Line 161: firstLevelDistanceSetList = new ArrayList<Pair<Integer, Set<VdsNumaNode>>>(); If this is final and initialized in construct, you can just clear here. Line 162: for (VdsNumaNode node : getNumaNodeList()) { Line 163: Map<Integer, Set<VdsNumaNode>> distances = new HashMap<Integer, Set<VdsNumaNode>>(); Line 164: for (Entry<Integer, Integer> entry : node.getNumaNodeDistances().entrySet()) { Line 165: Set<VdsNumaNode> sameDistanceNodes = distances.get(entry.getValue()); Line 209: return numaNodeList; Line 210: } Line 211: Line 212: public void setNumaNodeList(List<VdsNumaNode> numaNodeList) { Line 213: this.numaNodeList = numaNodeList; Maybe verify that this is not null, if it is null set it to the empty list? Otherwise the init loop might fail. Also you can populate the index node map here. Line 214: } Line 215: Line 216: public List<VM> getVmsWithvNumaNodeList() { Line 217: return vmsWithvNumaNodeList; http://gerrit.ovirt.org/#/c/28299/1/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/host/numa/NumaItemPanel.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/host/numa/NumaItemPanel.java: Line 57: public NumaItemPanel(VNodeModel item, boolean draggable) { Line 58: this.draggable = draggable; Line 59: getElement().setDraggable(draggable ? Element.DRAGGABLE_TRUE : Element.DRAGGABLE_FALSE); Line 60: Line 61: dragImage.setVisible(true); should this be as you only want the image visible if you can drag: dragImage.setVisible(draggable);? Line 62: Image editImage = new Image(resources.editHover()); Line 63: actionButton = new PushButton(editImage, new ClickHandler() { Line 64: Line 65: @Override Line 142: if (draggable) { Line 143: addBitlessDomHandler(new DragStartHandler() { Line 144: @Override Line 145: public void onDragStart(DragStartEvent event) { Line 146: NumaItemPanel sourcePanel = (NumaItemPanel) event.getSource(); I am pretty sure that you can't be guaranteed that the source is a NumeItemPanel, so you should check for it. Line 147: // Required: set data for the event. Line 148: lastDragData = Line 149: sourcePanel.item.getVm().getName() + " " + sourcePanel.item.getVmNumaNode().getIndex(); //$NON-NLS-1$ Line 150: event.setData("Text", lastDragData); //$NON-NLS-1$ -- To view, visit http://gerrit.ovirt.org/28299 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0712ea82b87d4fe9fc25f196183136485c0c4381 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com> Gerrit-Reviewer: Einav Cohen <eco...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@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
