Arik Hadas has posted comments on this change. Change subject: core: introduce convert vm command ......................................................................
Patch Set 94: (7 comments) https://gerrit.ovirt.org/#/c/33055/94/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ConvertVmCallback.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ConvertVmCallback.java: Line 23: } Line 24: Line 25: @Override Line 26: public void doPolling(Guid cmdId, List<Guid> childCmdIds) { Line 27: V2VJobInfo jobInfo = getV2VJobInfo(); > can this return null? do we want/need to handle it? No, getV2VJobInfo can not return null. we set the initial state of the job and once we remove it, no future polling should be done Line 28: switch (jobInfo.getStatus()) { Line 29: case STARTING: Line 30: case COPYING_DISK: Line 31: break; Line 57: getCommand().endAction(); Line 58: } Line 59: Line 60: private V2VJobInfo getV2VJobInfo() { Line 61: ConvertVmCommand<?> command = getCommand(); > same question here No, cannot be null Line 62: return command.getVdsManager().getV2VJobInfoForVm(command.getVmId()); Line 63: } Line 64: Line 65: private ConvertVmCommand<ConvertVmParameters> getCommand() { https://gerrit.ovirt.org/#/c/33055/94/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ConvertVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ConvertVmCommand.java: Line 55: } Line 56: Line 57: @Override Line 58: protected void init() { Line 59: setVmId(getParameters().getVmId()); > this is part of the VmCommand ctor, so not really needed Done Line 60: setVmName(getParameters().getVmName()); Line 61: setVdsId(getParameters().getProxyHostId()); Line 62: setVdsGroupId(getParameters().getVdsGroupId()); Line 63: setStoragePoolId(getParameters().getStoragePoolId()); Line 109: @Override Line 110: public AuditLogType getAuditLogTypeValue() { Line 111: switch (getActionState()) { Line 112: case EXECUTE: Line 113: return AuditLogType.IMPORTEXPORT_STARTING_CONVERT_VM; > even if succeeded == false? Done Line 114: case END_SUCCESS: Line 115: if (getSucceeded()) { Line 116: return AuditLogType.IMPORTEXPORT_IMPORT_VM; Line 117: } Line 197: } Line 198: Line 199: @Override Line 200: protected void endWithFailure() { Line 201: auditLog(this, AuditLogType.IMPORTEXPORT_CONVERT_FAILED); > wouldnt it cause 2 audit logs? (this one here and another by the commandBas yes, there will be 2 audit logs in that case 1. this one that indicates that the convert failed 2. the one from getAuditLogTypeValue that indicates the import operation failed I think this extra information for indicating that the conversion was the part that failed might be informative for users Line 202: removeVm(); Line 203: deleteV2VJob(); Line 204: setSucceeded(true); Line 205: } Line 200: protected void endWithFailure() { Line 201: auditLog(this, AuditLogType.IMPORTEXPORT_CONVERT_FAILED); Line 202: removeVm(); Line 203: deleteV2VJob(); Line 204: setSucceeded(true); > please comment why you set succeeded to true on failure there is no special reason. we need to set it to true so there will be no retry. actually this is the default setting in the commands framework, look at CommandBase#endWithFailure Line 205: } Line 206: Line 207: private VM readVmFromOvf(String ovf) { Line 208: try { https://gerrit.ovirt.org/#/c/33055/94/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/ConvertVmVDSParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/ConvertVmVDSParameters.java: Line 79: } Line 80: Line 81: public void setVmId(Guid vmId) { Line 82: this.vmId = vmId; Line 83: } > please print params (appendAttributes method) Done -- To view, visit https://gerrit.ovirt.org/33055 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7bc67ff237d5c01fc5f3c9f21c822573e5db32a3 Gerrit-PatchSet: 94 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Arik Hadas <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: Shahar Havivi <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
