Piotr Kliczewski has posted comments on this change. Change subject: events: removing vm.getStats from event processing ......................................................................
Patch Set 10: (7 comments) https://gerrit.ovirt.org/#/c/40205/10/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmDynamic.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmDynamic.java: Line 75: private Map<GraphicsType, GraphicsInfo> graphicsInfos; Line 76: private Long guestMemoryCached; Line 77: private Long guestMemoryBuffered; Line 78: private Long guestMemoryFree; Line 79: private Double statusUpdatedTime; > why not long? ElapsedTime is a Double so I wanted to be consistent. There is no time change issue because we are using monotonic time. Line 80: private String guestOsVersion; Line 81: private String guestOsDistribution; Line 82: private String guestOsCodename; Line 83: private ArchitectureType guestOsArch; https://gerrit.ovirt.org/#/c/40205/10/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VmAnalyzer.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VmAnalyzer.java: Line 645: : VMStatus.MigratingTo; Line 646: Line 647: // handing over the VM to the DST by marking it running on it. it will now be its SRC host. Line 648: VdsManager manager = vmsMonitoring.getResourceManager().GetVdsManager(vmToRemove.getRunOnVds()); Line 649: manager.resetStatusUpdateTime(vmToRemove.getId()); > you can just use getVdsManager() Done Line 650: vmToRemove.setRunOnVds(destinationHostId); Line 651: Line 652: log.info("Handing over VM '{}'({}) to Host '{}'. Setting VM to status '{}'", Line 653: vmToRemove.getName(), Line 697: // is not MigratingFrom, it means the migration failed Line 698: if (oldVmStatus == VMStatus.MigratingFrom && currentVmStatus != VMStatus.MigratingFrom Line 699: && currentVmStatus.isRunning()) { Line 700: VdsManager manager = vmsMonitoring.getResourceManager().GetVdsManager(vmToUpdate.getRunOnVds()); Line 701: manager.resetStatusUpdateTime(vmToUpdate.getId()); > same Done Line 702: Line 703: rerun = true; Line 704: log.info("Adding VM '{}' to re-run list", runningVm.getId()); Line 705: vmToUpdate.setMigratingToVds(null); https://gerrit.ovirt.org/#/c/40205/10/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/jsonrpc/EventVMStatsRefresher.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/jsonrpc/EventVMStatsRefresher.java: Line 76: @SuppressWarnings("unchecked") Line 77: private List<VmInternalData> convertEvent(Map<String, Object> map) { Line 78: List<VmInternalData> returnVMs = new ArrayList<VmInternalData>(); Line 79: for (Map.Entry<String, Object> entry : map.entrySet()) { Line 80: if (VdsProperties.notify_time.equals(entry.getKey())) { > this line is not clear, maybe worth a comment Done Line 81: continue; Line 82: } Line 83: VmDynamic vmdynamic = new VmDynamic(); Line 84: vmdynamic.setId(new Guid((String) entry.getKey())); Line 91: return returnVMs; Line 92: } Line 93: Line 94: @SuppressWarnings("unchecked") Line 95: private void prepareChanges(List<VmInternalData> vms, List<Pair<VM, VmInternalData>> changedVms, > i think that this method can be merged with convertEvent method, i dont und This code is based on original code but we can refactor it. Line 96: List<Pair<VM, VmInternalData>> devicesChangedVms, Map<String, Object> map) { Line 97: Double notifyTime = parseDouble(map.get(VdsProperties.notify_time)); Line 98: Line 99: for (VmInternalData vdsmVm : vms) { Line 103: VmInternalData vmData = createVmInternalData(dbVm, (Map<String, Object>) map.get(vmid.toString()), notifyTime); Line 104: Line 105: // make sure to ignore events from other hosts during migration Line 106: // and process once the migration is done Line 107: if (dbVm.getRunOnVds() == null > not sure you can assume dbVm != null it is null when it is created (WaitForLaunch) Line 108: || dbVm.getRunOnVds().equals(manager.getVdsId()) Line 109: || (!dbVm.getRunOnVds().equals(manager.getVdsId()) && vmData.getVmDynamic().getStatus() == VMStatus.Up)) { Line 110: if (vmData != null) { Line 111: changedVms.add(new Pair<>(dbVm, vmData)); Line 117: } Line 118: } Line 119: Line 120: private VmInternalData createVmInternalData(VM dbVm, Map<String, Object> xmlRpcStruct, Double notifyTime) { Line 121: VmDynamic vmDynamic = new VmDynamic(); > the idea was to base the vdsmVm with the vm from db, and set on it the new We can base it on db values. We need to add copying constructor. Line 122: if (xmlRpcStruct.containsKey(VdsProperties.status)) { Line 123: vmDynamic.setStatus(convertToVmStatus((String) xmlRpcStruct.get(VdsProperties.status))); Line 124: } Line 125: -- To view, visit https://gerrit.ovirt.org/40205 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib15df9ce78bbb86c9284bdd6b772d0e6801db765 Gerrit-PatchSet: 10 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek <mskri...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
