Arik Hadas has posted comments on this change. Change subject: engine: clear the pending memory when destroying paused VM ......................................................................
Patch Set 4: (3 comments) http://gerrit.ovirt.org/#/c/25271/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/StopVmCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/StopVmCommandBase.java: Line 87: } Line 88: Line 89: //need to check the Paused status before we destroy the VM Line 90: if (getVm().getStatus() == VMStatus.Paused && getVm().getVmPauseStatus() == VmPauseStatus.NOERR) { Line 91: wasPaused = true; 1. this is a matter of style but can you merge the declaration of wasPaused with the assignment in #90 like: boolean wasPaused = getVm().getStatus()... IMO it will make the code shorter and more readable 2. just a suggestion, for the sake of making the code more easy to understand, how about moving it to be before line 80 so it will be clear that we don't expect the status to be changed as a result of the first destroy command? Line 92: } Line 93: setActionReturnValue(Backend Line 94: .getInstance() Line 95: .getResourceManager() Line 95: .getResourceManager() Line 96: .RunVdsCommand(VDSCommandType.DestroyVm, Line 97: new DestroyVmVDSCommandParameters(getVdsId(), getVmId(), false, false, 0)).getReturnValue()); Line 98: Line 99: // if the VM was run paused, then we need to clean the pendingVms I would drop this comment, your code explains itself so no need to repeat in a comment Line 100: if (wasPaused) { Line 101: VmHandler.decreasePendingVms(getVm().getRunOnVds(), vmStatic.getNumOfCpus(), Line 102: vmStatic.getMinAllocatedMem(), vmStatic.getName()); Line 103: } Line 97: new DestroyVmVDSCommandParameters(getVdsId(), getVmId(), false, false, 0)).getReturnValue()); Line 98: Line 99: // if the VM was run paused, then we need to clean the pendingVms Line 100: if (wasPaused) { Line 101: VmHandler.decreasePendingVms(getVm().getRunOnVds(), vmStatic.getNumOfCpus(), why not just call VmHandler.decreasePendingVms(VM, Guid) with the getVm() (and then vmStatic won't be necessary) ? Line 102: vmStatic.getMinAllocatedMem(), vmStatic.getName()); Line 103: } Line 104: } Line 105: -- To view, visit http://gerrit.ovirt.org/25271 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1de34ac8b0b6e48d360161d8f0ff8d1fe4f6fd04 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Jiří Moskovčák <jmosk...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Kobi Ianko <k...@redhat.com> Gerrit-Reviewer: Martin Sivák <msi...@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
