Liron Ar has posted comments on this change. Change subject: engine: Remove address from hotunplugged devices ......................................................................
Patch Set 3: (1 comment) The code seems good to me, the issue that i see is a possible race with VdsUpdateRuntimeInfo which will lead us to the same situation eventually. http://gerrit.ovirt.org/#/c/26598/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HotUnPlugDiskFromVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HotUnPlugDiskFromVmCommand.java: Line 32: Line 33: @Override Line 34: protected void updateDeviceProperties() { Line 35: VmDevice device = getVmDeviceDao().get(oldVmDevice.getId()); Line 36: device.setIsPlugged(!oldVmDevice.getIsPlugged()); as we know that we are already in the hot unplug command, it can be just set to false (same goes for the parent command). but as the updateDeviceProperties() method is basically similar to the parent implementation except to the if clause, i'd suggest to have here only the uncommon logic. Line 37: if (getVm().getStatus().isRunningOrPaused()) { Line 38: device.setAddress(""); Line 39: } Line 40: getVmDeviceDao().updateHotPlugDisk(device); -- To view, visit http://gerrit.ovirt.org/26598 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id5850fd4c15a230dbc27e8c22d47935083da21c2 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Xavi Francisco <xfran...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@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
