Shahar Havivi has posted comments on this change. Change subject: core: notification to the user that a new client tools are available ......................................................................
Patch Set 3: (7 comments) http://gerrit.ovirt.org/#/c/36556/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmHandler.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmHandler.java: Line 958: // Line 959: public static void refreshVmsToolsVersion(Guid poolId, Set<String> isoList) { Line 960: List<VDSGroup> clusters = DbFacade.getInstance().getVdsGroupDao().getAllForStoragePool(poolId); Line 961: for (VDSGroup cluster: clusters) { Line 962: List<VM> vms = DbFacade.getInstance().getVmDao().getAllForVdsGroup(cluster.getId()); > how about adding a query to get all VMs in data-center? Done Line 963: for (VM vm : vms) { Line 964: if (vm.getAppList() != null && vm.getAppList().toLowerCase().contains("rhev-tools")) { Line 965: Matcher m = TOOLS_PATTERN.matcher(vm.getAppList().toLowerCase()); Line 966: if (m.matches() && m.groupCount() > 0) { Line 964: if (vm.getAppList() != null && vm.getAppList().toLowerCase().contains("rhev-tools")) { Line 965: Matcher m = TOOLS_PATTERN.matcher(vm.getAppList().toLowerCase()); Line 966: if (m.matches() && m.groupCount() > 0) { Line 967: GuestAgentStatus oldStatus = vm.getGuestAgentStatus(); Line 968: GuestAgentStatus currStatus = getAgentStatus(m.group(1), isoList); > we can retrieve the guest-agent version from the iso list only once and not Done Line 969: if (oldStatus != currStatus) { Line 970: vm.setGuestAgentStatus(currStatus); Line 971: DbFacade.getInstance().getVmDynamicDao().update(vm.getDynamicData()); Line 972: } Line 967: GuestAgentStatus oldStatus = vm.getGuestAgentStatus(); Line 968: GuestAgentStatus currStatus = getAgentStatus(m.group(1), isoList); Line 969: if (oldStatus != currStatus) { Line 970: vm.setGuestAgentStatus(currStatus); Line 971: DbFacade.getInstance().getVmDynamicDao().update(vm.getDynamicData()); > I'm concerned because of possible races like: Done Line 972: } Line 973: } Line 974: } Line 975: } Line 985: String isoVersion = m.group(1).replace('_', '.'); Line 986: if (toolsVersion.compareTo(isoVersion) < 0) { Line 987: return GuestAgentStatus.UpdateNeeded; Line 988: } else { Line 989: retVal = GuestAgentStatus.Exists; > can we return 'Exists' here or is there a reason to keep looping? no... since it can be a new version later in the iteration but its not relevant since I am changing the logic regarding comment #2 Line 990: } Line 991: } Line 992: } Line 993: } Line 991: } Line 992: } Line 993: } Line 994: return retVal; Line 995: } > please add unit-tests Added unit and dao tests http://gerrit.ovirt.org/#/c/36556/3/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/GuestAgentStatus.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/GuestAgentStatus.java: Line 5: Line 6: public enum GuestAgentStatus { Line 7: DoesntExist(0), Line 8: Exists(1), Line 9: UpdateNeeded(2); > why did you choose to use an integer and not its name? I prefer to use the We have a lot of status as the value, I prefer using int value because: 1. low on memory and performance (on each vm) 2. easier to rename the 'name' without effecting the DB Line 10: Line 11: private static final Map<Integer, GuestAgentStatus> mappings = new HashMap<Integer, GuestAgentStatus>(); Line 12: private int value; Line 13: http://gerrit.ovirt.org/#/c/36556/3/packaging/dbscripts/upgrade/03_06_0695_add_guest_agent_status_to_vm_dynamic.sql File packaging/dbscripts/upgrade/03_06_0695_add_guest_agent_status_to_vm_dynamic.sql: Line 1: -- add guest_agent_status Enum field to vm_dynamic Line 2: -- values can be: Line 3: -- 0 Exists Line 4: -- 1 DoesntExist > the comment is wrong: 0 - DoesntExist, 1 - Exists so it will be clear that Done Line 5: -- 2 UpdateNeeded -- To view, visit http://gerrit.ovirt.org/36556 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If098422eb76295517022de1c7f716db354e7853e Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@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
