Michal Skrivanek has posted comments on this change. Change subject: engine : command infrastructure should know when the "execute" phase finished ......................................................................
Patch Set 3: (7 comments) http://gerrit.ovirt.org/#/c/30281/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java: Line 1136: } Line 1137: functionReturnValue = getSucceeded(); Line 1138: exceptionOccurred = false; Line 1139: } catch (VdcBLLException e) { Line 1140: setCommandStatus(CommandStatus.FAILED); so the status is different from exceptionOccurred? why? If not then this should be in the "finally" section Line 1141: log.error(String.format("Command %1$s throw Vdc Bll exception. With error message %2$s", Line 1142: getClass().getName(), Line 1143: e.getMessage())); Line 1144: if (log.isDebugEnabled()) { http://gerrit.ovirt.org/#/c/30281/3/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 48: getReturnValue().getInternalVdsmTaskIdList().add(result); Line 49: getReturnValue().getVdsmTaskIdList().add(result); Line 50: getParameters().getVdsmTaskIds().add(result); Line 51: setSucceeded(vdsReturnValue.getSucceeded()); Line 52: setCommandStatus(CommandStatus.ACTIVE_ASYNC_EXECUTED); I'd first print the log.info below and then declare success Line 53: persistCommandWithContext(getParameters().getParentCommand(), true); Line 54: log.info("Successfully started task to remove orphaned volumes resulting from live merge"); Line 55: } else { Line 56: setSucceeded(false); http://gerrit.ovirt.org/#/c/30281/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotCommand.java: Line 141: Line 142: setSucceeded(true); Line 143: if (snapshotHasImages) { Line 144: if (getSnapshotActionType() == VdcActionType.RemoveSnapshotSingleDiskLive) { Line 145: setCommandStatus(CommandStatus.ACTIVE_ASYNC_EXECUTED); so what sets the commandstatus on the other action types? Line 146: } Line 147: } Line 148: } Line 149: http://gerrit.ovirt.org/#/c/30281/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotCommandCallback.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveSnapshotCommandCallback.java: Line 21: case ACTIVE: Line 22: case ACTIVE_SYNC: Line 23: case ACTIVE_ASYNC: Line 24: case ACTIVE_ASYNC_EXECUTED: Line 25: log.info("Waiting on Live Merge child commands to complete"); so we get this every 10s for each and every snapshot? Line 26: return; Line 27: case FAILED: Line 28: case FAILED_RESTARTED: Line 29: case UNKNOWN: http://gerrit.ovirt.org/#/c/30281/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/CommandExecutor.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/CommandExecutor.java: Line 80: for (CommandEntity cmdEntity : coco.getCommandsWithCallBackEnabled()) { Line 81: switch(cmdEntity.getCommandStatus()) { Line 82: case ACTIVE_SYNC: Line 83: case ACTIVE_ASYNC: Line 84: case NOT_STARTED: isn't it missing "ACTIVE"? Line 85: coco.retrieveCommand(cmdEntity.getId()).setCommandStatus(CommandStatus.FAILED_RESTARTED); Line 86: break; Line 87: default: Line 88: if (!cmdEntity.isCallBackNotified()) { http://gerrit.ovirt.org/#/c/30281/3/backend/manager/modules/compat/src/main/java/org/ovirt/engine/core/compat/CommandStatus.java File backend/manager/modules/compat/src/main/java/org/ovirt/engine/core/compat/CommandStatus.java: overall this deserves better description for each status Line 1: package org.ovirt.engine.core.compat; Line 2: Line 3: public enum CommandStatus { Line 4: UNKNOWN, Line 4: UNKNOWN, Line 5: NOT_STARTED, Line 6: ACTIVE, // the execute methods on command base has been invoked Line 7: ACTIVE_SYNC, // used by synchronous commands to indicate that the sync command is executing Line 8: ACTIVE_ASYNC, // used by async commands to indicate that async command has started executing why do we have ACTIVE then? is that supposed to be a superset of these? If we need to have a clear distinction between sync and async then maybe better remove it? Line 9: ACTIVE_ASYNC_EXECUTED, // used by async commands to indicate that async command execute method has completed Line 10: FAILED, Line 11: FAILED_RESTARTED, // set by command executor to indicate that the sync command did not complete Line 12: // and the server was restarted -- To view, visit http://gerrit.ovirt.org/30281 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie36e7d8a0263d5dd90fabe914a76711d5c001e72 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Greg Padgett <gpadg...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@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
