Omer Frenkel has posted comments on this change.

Change subject: events: VM Status based on an event
......................................................................


Patch Set 19:

(2 comments)

https://gerrit.ovirt.org/#/c/37488/19/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/RefresherFactory.java
File 
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/RefresherFactory.java:

Line 12: public class RefresherFactory {
Line 13: 
Line 14:     public static VMStatsRefresher create(final VdsManager manager, 
final AuditLogDirector auditLogDirector, final SchedulerUtil scheduler) {
Line 15:         Version version = manager.getCompatibilityVersion();
Line 16:         if (FeatureSupported.jsonProtocol(version) && 
FeatureSupported.vmStatsEvents(version)) {
> Jsonrpc is enabled per host and it is available for hosts 3.5+. This check 
ok so its just makes my comment valid, we can have 3.6 cluster with host that 
have json disabled, so FeatureSupported.jsonProtocol(version) will return true 
but user is not using json, so we probably need to check also 
host.getVdsProtocol
Line 17:             return new EventVMStatsRefresher(manager, 
auditLogDirector, scheduler);
Line 18:         }
Line 19:         return new PollVMStatsRefresher(manager, auditLogDirector, 
scheduler);
Line 20:     }


https://gerrit.ovirt.org/#/c/37488/19/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 89:                     VmInternalData vmData = fetchStats(dbVm, vdsmVm);
Line 90:                     if (vmData != null) {
Line 91:                         changedVms.add(new Pair<>(dbVm, vmData));
Line 92:                     }
Line 93:                     if (isDevicesChanged(dbVm, vdsmVm)) {
> The order was based on original code we compare it with dbVms so I think it
ok, i see, its a bug but it exists in the original code as well
Line 94:                         devicesChangedVms.add(new Pair<>(dbVm, 
vdsmVm));
Line 95:                     }
Line 96:                 }
Line 97:             }


-- 
To view, visit https://gerrit.ovirt.org/37488
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If5b35877ccd63372759ad6989280e9417c259b21
Gerrit-PatchSet: 19
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Piotr Kliczewski <[email protected]>
Gerrit-Reviewer: Arik Hadas <[email protected]>
Gerrit-Reviewer: Barak Azulay <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Francesco Romani <[email protected]>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <[email protected]>
Gerrit-Reviewer: Michal Skrivanek <[email protected]>
Gerrit-Reviewer: Moti Asayag <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Oved Ourfali <[email protected]>
Gerrit-Reviewer: Piotr Kliczewski <[email protected]>
Gerrit-Reviewer: Roy Golan <[email protected]>
Gerrit-Reviewer: [email protected]
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to