Arik Hadas has posted comments on this change. Change subject: engine: calling CollectVdsNetworkData after vm state changed to up ......................................................................
Patch Set 23: (3 comments) https://gerrit.ovirt.org/#/c/38261/23/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/hostdev/HostDeviceManager.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/hostdev/HostDeviceManager.java: Line 61: for (Guid vmId : vmIds) { Line 62: shouldRefresh = checkVmNeedsHostDevices(vmId); Line 63: Line 64: if (shouldRefresh) { Line 65: continue; there's a bug here because if the last VM doesn't need host devices you won't refresh.. you probably meant to break Line 66: } Line 67: } Line 68: Line 69: if (shouldRefresh) { Line 66: } Line 67: } Line 68: Line 69: if (shouldRefresh) { Line 70: backend.runInternalAction(VdcActionType.RefreshHost, new VdsActionParameters(hostId)); I think this method could be simplified: public void refreshHostIfAnyVmHasHostDevices(...) { for (Guid vmId : vmIds) { if (checkVmNeedsHostDevices(vmId)) { backend. runInternalAction(VdcActionType.RefreshHost, new VdsActionParameters(hostId)); return; } } } no? (I saw you've been asked to keep return values as variables so you can keep the result of checkVmNeedsHostDevices in some boolean field, it doesn't matter to me) Line 71: } Line 72: } https://gerrit.ovirt.org/#/c/38261/23/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VmsMonitoring.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/VmsMonitoring.java: Line 261: .getEventListener() Line 262: .refreshHostIfAnyVmHasHostDevices(vmIds, vdsManager.getVdsId()); Line 263: } Line 264: }); Line 265: } is there a reason for having this code here and not in VdsEventListener like the rest of the similar stuff? Line 266: Line 267: private void processVmsWithDevicesChange() { Line 268: // Handle VM devices were changed (for 3.1 cluster and above) Line 269: if (!VmDeviceCommonUtils.isOldClusterVersion(vdsManager.getGroupCompatibilityVersion())) { -- To view, visit https://gerrit.ovirt.org/38261 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id89c51e738b75af0492e3b314a261b6beba7ee76 Gerrit-PatchSet: 23 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@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
