Omer Frenkel has posted comments on this change. Change subject: core: introduce convert vm command ......................................................................
Patch Set 94: (8 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? 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 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 Line 60: setVmName(getParameters().getVmName()); Line 61: setVdsId(getParameters().getProxyHostId()); Line 62: setVdsGroupId(getParameters().getVdsGroupId()); Line 63: setStoragePoolId(getParameters().getStoragePoolId()); Line 70: cachedCallback = new ConvertVmCallback(getCommandId()); Line 71: // if the callback is created after the command was executed, it means that the engine restarted Line 72: // so there is no v2v-job in vdsManager and thus we add a new job with unknown status there Line 73: if (getCommandExecutionStatus() == CommandExecutionStatus.EXECUTED) { Line 74: getVdsManager().addV2VJobInfoForVm(getVmId(), JobStatus.UNKNOWN); why we are moving to procedural programming? Line 75: } Line 76: } Line 77: return cachedCallback; Line 78: } 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? 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 commandBase (which will log it was succeeded because succeeded = true)? 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 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) -- 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 <aha...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek <mskri...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
