Piotr Kliczewski has posted comments on this change.

Change subject: events: removing vm.getStats from event processing
......................................................................


Patch Set 17:

(5 comments)

https://gerrit.ovirt.org/#/c/40205/17/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 79:                     VM dbVm = dbFacade.getVmDao().get(vmid);
Line 80: 
Line 81:                     VmInternalData vdsmVm = createVmInternal(entry, 
dbVm);
Line 82: 
Line 83:                     VmInternalData vmData = createVmInternalData(dbVm, 
(Map<String, Object>) map.get(vmid.toString()), notifyTime);
> i still think we should merge createVmInternal and createVmInternalData to 
Done
Line 84:                     // make sure to ignore events from other hosts 
during migration
Line 85:                     // and process once the migration is done
Line 86:                     if (dbVm != null && (dbVm.getRunOnVds() == null
Line 87:                             || 
dbVm.getRunOnVds().equals(manager.getVdsId())


Line 84:                     // make sure to ignore events from other hosts 
during migration
Line 85:                     // and process once the migration is done
Line 86:                     if (dbVm != null && (dbVm.getRunOnVds() == null
Line 87:                             || 
dbVm.getRunOnVds().equals(manager.getVdsId())
Line 88:                             || 
(!dbVm.getRunOnVds().equals(manager.getVdsId()) && 
vmData.getVmDynamic().getStatus() == VMStatus.Up))) {
> if dbVm is null we will get exception on the second part of the 'if'
we have short circuit check for dbVm being null.
Line 89:                         if (vmData != null) {
Line 90:                             changedVms.add(new Pair<>(dbVm, vmData));
Line 91:                         }
Line 92:                         if (isDevicesChanged(dbVm, vdsmVm)) {


Line 97:             }
Line 98: 
Line 99:             @SuppressWarnings("unchecked")
Line 100:             private VmInternalData createVmInternal(Map.Entry<String, 
Object> entry, VM dbVm) {
Line 101:                 VmDynamic vmdynamic = new 
VmDynamic(dbVm.getDynamicData());
> db vm can be null (in case of external vms)
I will move the check higher.
Line 102:                 vmdynamic.setId(new Guid((String) entry.getKey()));
Line 103:                 Map<String, Object> stats = (Map<String, Object>) 
entry.getValue();
Line 104:                 vmdynamic.setStatus(convertToVmStatus((String) 
stats.get(VdsProperties.status)));
Line 105:                 return new VmInternalData(vmdynamic, null, null, 
null);


Line 105:                 return new VmInternalData(vmdynamic, null, null, 
null);
Line 106:             }
Line 107: 
Line 108:             private VmInternalData createVmInternalData(VM dbVm, 
Map<String, Object> xmlRpcStruct, Double notifyTime) {
Line 109:                 VmDynamic vmDynamic = new VmDynamic();
> this also should be based on dbVm because this is what we send to the monit
OK and as above.
Line 110:                 if (xmlRpcStruct.containsKey(VdsProperties.status)) {
Line 111:                     vmDynamic.setStatus(convertToVmStatus((String) 
xmlRpcStruct.get(VdsProperties.status)));
Line 112:                 }
Line 113: 


Line 131:                 } else {
Line 132:                     vmDynamic.setExitReason(VmExitReason.Unknown);
Line 133:                 }
Line 134: 
Line 135:                 return new VmInternalData(vmDynamic, 
dbVm.getStatisticsData(), null, new HashMap<String, LUNs>());
> dbVm can be null
as above.
Line 136:             }
Line 137: 
Line 138:             private Double parseDouble(Object value) {
Line 139:                 if (Long.class.isInstance(value)) {


-- 
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: 17
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

Reply via email to