Arik Hadas 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" seems you're right, I'll change 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? The parent parameters? because we want the RemoveVmCommand to poll those tasks. 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? t the removeMemoryVolume method will be invoked for each memory volume. when you're doing A() & B(), A and B will always be invoked, and R &= A() is like R = R & A() thus A will always be invoked. it doesn't make sense to change it as you suggest.. 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 fr I prefer to keep it that way because that way it is clear on which flow we are. if we'll have additional flows that will require to generalize this, we'll do it then. I don't understand the second part ("export the db load") - what do you mean? 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
