Liron Ar has posted comments on this change. Change subject: core: managed removal of memory volumes in negative flows ......................................................................
Patch Set 3: (4 comments) http://gerrit.ovirt.org/#/c/23222/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java: Line 1178: private void removeMemoryVolumes(String memoryVolume, Guid vmId) { Line 1179: VdcReturnValueBase retVal = getBackend().runInternalAction( Line 1180: VdcActionType.RemoveMemoryVolumes, Line 1181: new RemoveMemoryVolumesParameters(memoryVolume, vmId), Line 1182: ExecutionHandler.createDefaultContexForTasks(getExecutionContext())); seems like we don't need "createDefaultContexForTasks" Line 1183: Line 1184: if (!retVal.getSucceeded()) { Line 1185: log.errorFormat("Failed to remove memory volumes: {0}", memoryVolume); Line 1186: } http://gerrit.ovirt.org/#/c/23222/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmCommand.java: Line 145: private RemoveMemoryVolumesParameters buildRemoveMemoryVolumesParameters(String memoryState, Guid vmId) { Line 146: RemoveMemoryVolumesParameters parameters = Line 147: new RemoveMemoryVolumesParameters(memoryState, getVmId()); Line 148: parameters.setParentCommand(getActionType()); Line 149: parameters.setParentParameters(getParameters()); why do we need this here? Line 150: parameters.setEntityInfo(getParameters().getEntityInfo()); Line 151: Line 152: return parameters; Line 153: } http://gerrit.ovirt.org/#/c/23222/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/MemoryImageRemover.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/MemoryImageRemover.java: Line 72: */ Line 73: protected boolean removeMemoryVolumes(Set<String> memoryVolumes) { Line 74: boolean allVolumesRemovedSucessfully = true; Line 75: for (String memoryVols : memoryVolumes) { Line 76: allVolumesRemovedSucessfully &= removeMemoryVolume(memoryVols); if we failed to remove one volume we won't try to remove the others, why? this line should be : allVolumesRemovedSucessfully = removeMemoryVolume(memoryVols) & allVolumesRemovedSucessfully Line 77: } Line 78: Line 79: return allVolumesRemovedSucessfully; Line 80: } http://gerrit.ovirt.org/#/c/23222/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/MemoryImageRemoverOnDataDomain.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/memory/MemoryImageRemoverOnDataDomain.java: Line 51: Line 52: // from RemoveVmCommand this command is called after the snapshot Line 53: // is removed from the DB Line 54: return enclosingCommand.getParameters().getParentCommand() == VdcActionType.RemoveVm ? Line 55: getSnapshotDao().getNumOfSnapshotsByMemory(memoryVolume) == 0 IMO MemoryImageRemoverOnDataDomain shouldbn't be aware of being executed from commands, what if we'll want to run it from internal flow? this should be passed in the parameters imo. btw, if you can please export the db load to a line before. Line 56: : getSnapshotDao().getNumOfSnapshotsByMemory(memoryVolume) == 1; Line 57: } Line 58: Line 59: protected SnapshotDao getSnapshotDao() { -- To view, visit http://gerrit.ovirt.org/23222 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib4b270ec0e1ab41cae34459dde9f9cf47b1b5bdf Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Arik Hadas <aha...@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: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> 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
