Ravi Nori has posted comments on this change. Change subject: engine : command infrastructure should know when the "execute" phase finished ......................................................................
Patch Set 3: (6 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 sh Will do 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 Will do 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); > Michal is right. But not all commands are async commands, some are sync. Only Async commands should status to ACTIVE_ASYNC when execution begins and ACTIVE_ASYNC_EXECUTED when execution ends 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? We don't have event driven notification yet, until we have that this is going to be executed every 10 seconds 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"? Commands that use CommandExecutor framework should set status to ACTIVE_SYNC/ACTIVE_ASYNC and finally ACTIVE_ASYNC_EXECUTED. We are not managing commands that are old style in CommandExecutor which sets status to 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: 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 This is set in CommandBase for commands that just needs status of NOT_STARTED/ACTIVE/SUCCEEDED/FAILED and not managed by CommandExecutor 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 <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Greg Padgett <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Ravi Nori <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
