Tomas Jelinek has posted comments on this change. Change subject: userportal\webadmin: cluster parameters override ......................................................................
Patch Set 16: (4 comments) http://gerrit.ovirt.org/#/c/31582/16/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/RunOnceModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/RunOnceModel.java: Line 1007: if (returnValue != null) { Line 1008: Set<String> emulatedSet = new TreeSet<String>((HashSet<String>) returnValue); Line 1009: String oldVal = getEmulatedMachine().getSelectedItem(); Line 1010: getEmulatedMachine().setItems(emulatedSet); Line 1011: getEmulatedMachine().setSelectedItem(oldVal); no need to check if the oldVal is not empty? Line 1012: } Line 1013: } Line 1014: }), clusterId); Line 1015: Line 1037: // replace 'cluster cpu' with the explicit run-time value Line 1038: if (StringHelper.isNullOrEmpty(oldVal) Line 1039: && !cpuList.isEmpty()) { Line 1040: getCustomCpu().setSelectedItem(cpuList.get( Line 1041: cpuList.size() - 1)); why the last and not the first? Line 1042: } else { Line 1043: getCustomCpu().setSelectedItem(oldVal); Line 1044: } Line 1045: } Line 1039: && !cpuList.isEmpty()) { Line 1040: getCustomCpu().setSelectedItem(cpuList.get( Line 1041: cpuList.size() - 1)); Line 1042: } else { Line 1043: getCustomCpu().setSelectedItem(oldVal); you want to call this only if the oldVal is not empty, I'd say Line 1044: } Line 1045: } Line 1046: } Line 1047: }), cluster.getcpu_name()); Line 1049: Line 1050: // replace 'cluster emulated machine' with the explicit run-time value Line 1051: if (StringHelper.isNullOrEmpty(getEmulatedMachine().getSelectedItem()) Line 1052: && cluster.getEmulatedMachine() != null) { Line 1053: getEmulatedMachine().setSelectedItem(cluster.getEmulatedMachine()); You have a race here - there is no guarantee that this will be called after the getEmulatedMachinesByClusterID() will return and set the emulated machines up. Mostly yes since this are two calls and that one is just one, but it is still not deterministic. You can extract this calls into a separate method and call them from getEmulatedMachinesByClusterID's callback. Line 1054: } Line 1055: } Line 1056: } Line 1057: }), clusterId); -- To view, visit http://gerrit.ovirt.org/31582 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia288e46f38c03935ec9b0817833b1921f15ec9f8 Gerrit-PatchSet: 16 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eldan Shachar <eshac...@redhat.com> Gerrit-Reviewer: Eldan Shachar <eshac...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@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
