Yair Zaslavsky has posted comments on this change. Change subject: core: introduce RemoveSnapshotSingleDiskLive BLL command ......................................................................
Patch Set 10: (8 comments) http://gerrit.ovirt.org/#/c/26909/10/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DestroyImageCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DestroyImageCommand.java: Line 39: VDSReturnValue vdsReturnValue = runVdsCommand(VDSCommandType.DestroyImage, Line 40: createVDSParameters()); Line 41: Line 42: if (vdsReturnValue != null && vdsReturnValue.getCreationInfo() != null) { Line 43: // TODO GP should we add a string message for createTask? what did you mean here? Line 44: getReturnValue().getInternalVdsmTaskIdList().add( Line 45: createTask(taskId, vdsReturnValue.getCreationInfo(), VdcActionType.DestroyImage, Line 46: VdcObjectType.Storage, getParameters().getStorageDomainId())); Line 47: getParameters().setVdsmTaskIds(new ArrayList<Guid>(Arrays.asList(taskId))); http://gerrit.ovirt.org/#/c/26909/10/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MergeStatusCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MergeStatusCommand.java: Line 27: private static final Log log = LogFactory.getLog(MergeStatusCommand.class); Line 28: Line 29: @Override Line 30: protected void executeCommand() { Line 31: log.error("GP LOG MergeStatusCommand.executeCommand() start"); Please fix the GP stuff :) Line 32: setCommandStatus(CommandStatus.ACTIVE_SYNC); Line 33: if (DbFacade.getInstance().getVmDao().get(getVmId()).isDown()) { Line 34: log.error("Failed to live merge, VM is not running"); Line 35: setCommandStatus(CommandStatus.FAILED); Line 45: } else { Line 46: // bypass doPolling(): we already know it failed Line 47: setCommandStatus(CommandStatus.FAILED); Line 48: } Line 49: log.error("GP LOG MergeStatusCommand.executeCommand() finish"); Same. Line 50: } Line 51: Line 52: private Set<Guid> getVolumeChain() { Line 53: List<String> vmIds = new ArrayList<>(); Line 61: } Line 62: Line 63: Map vm = (Map) vms[0]; Line 64: if (vm == null || vm.get(VdsProperties.vm_guid) == null) { Line 65: log.error("Received NULL VM or VM id when retrieving VM information, abort."); Please rephrase... what is a null VM? How about recieved no VM or VM id? Line 66: return null; Line 67: } Line 68: Line 69: Guid vmId = new Guid((String) vm.get(VdsProperties.vm_guid)); http://gerrit.ovirt.org/#/c/26909/10/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MergeStatusCommandCallback.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MergeStatusCommandCallback.java: Line 20: private static final Log log = LogFactory.getLog(MergeStatusCommand.class); Line 21: Line 22: @Override Line 23: public void doPolling(Guid cmdId, List<Guid> childCmdIds) { Line 24: log.error("GP LOG MergeStatusCommandCallback.doPolling() start"); Please fix GP log :) Line 25: MergeStatusCommand<MergeParameters> command = getCommand(cmdId); Line 26: Set<Guid> images = command.getParameters().getVmVolumeChain(); Line 27: Line 28: if (images == null) { Line 37: log.error("Failed to live merge, VM is not running"); Line 38: command.setCommandStatus(CommandStatus.FAILED); Line 39: } Line 40: Line 41: Set<Guid> imagesToRemove = getImagesToRemovePlusOne(command); IMHO strange method name... (PlusOne) Line 42: images.retainAll(imagesToRemove); Line 43: if (images.size() != 1) { Line 44: log.errorFormat("Failed to live merge, still in volume chain: {0}", images.toString()); Line 45: command.setCommandStatus(CommandStatus.FAILED); Line 68: * out the merge direction and preserve the appropriate image. Line 69: * Line 70: * @return Set of Guids representing the images which may be removed Line 71: */ Line 72: public Set<Guid> getImagesToRemovePlusOne(MergeStatusCommand<MergeParameters> command) { With such a nice comment - feel free to remove the "PlusOne" from the method name. Line 73: Set<Guid> imagesToRemove = new HashSet<>(); Line 74: DiskImage curr = command.getParameters().getTopImage(); Line 75: imagesToRemove.add(curr.getImageId()); Line 76: while (!curr.getParentId().equals(command.getParameters().getBaseImage().getParentId())) { http://gerrit.ovirt.org/#/c/26909/10/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotSingleDiskLiveCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotSingleDiskLiveCommand.java: Line 67: vdcReturnValue = TaskManagerUtil.getCommandReturnValue(childCommandIds.get(0)); Line 68: if (vdcReturnValue != null && vdcReturnValue.getSucceeded()) { Line 69: getParameters().setCommandStep(getParameters().getNextCommandStep()); Line 70: // TODO GP Seems likely to cause a race if server restarts at a bad time. Line 71: // TODO GP In fact, so does the step progression code below. Need to Any conclusion here? Line 72: // TODO GP define better semantics for stepping through child commands so Line 73: // TODO GP that interruption at any point is recoverable. Line 74: TaskManagerUtil.removeCommand(childCommandIds.get(0)); Line 75: break; -- To view, visit http://gerrit.ovirt.org/26909 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic47eb91a0ea1fe150e3b2152e2c9d5f1f2eb3678 Gerrit-PatchSet: 10 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Greg Padgett <gpadg...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Greg Padgett <gpadg...@redhat.com> Gerrit-Reviewer: Liron Ar <lara...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@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
