Kobi Ianko has posted comments on this change. Change subject: core: New internal command, VmSlaPolicyCommand ......................................................................
Patch Set 4: (6 comments) http://gerrit.ovirt.org/#/c/27646/4/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmSlaPolicyCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmSlaPolicyCommand.java: Line 38: setVdsId((Guid) getParameters().getSlaParameter(VmSlaPolicyParameters.Params.VDS_ID)); Line 39: setVdsGroupId(getVds().getVdsGroupId()); Line 40: if (!FeatureSupported.vmSlaPolicy(getVdsGroup().getcompatibility_version())) { Line 41: addCanDoActionMessage(VdcBllMessages.VM_SLA_POLICY_NOT_SUPPORTED); Line 42: return false; > why not return failCanDoAction here as well? Done Line 43: } Line 44: Line 45: if (getVm() == null) { Line 46: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_NOT_EXIST); Line 52: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VM_STATUS_ILLEGAL, Line 53: LocalizedVmStatus.from(getVm().getStatus())); Line 54: } Line 55: Line 56: return canDo; > please remove the canDo field and just return true here Done Line 57: } Line 58: Line 59: /** Line 60: * Execution shall perform a call to VDSM to set the SLA parameters. http://gerrit.ovirt.org/#/c/27646/4/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VmSlaPolicyParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VmSlaPolicyParameters.java: Line 3: import java.util.HashMap; Line 4: import java.util.Map; Line 5: import org.ovirt.engine.core.compat.Guid; Line 6: Line 7: public class VmSlaPolicyParameters extends VmManagementParametersBase { > I'm not sure json will be able to serialize/deserialize this class well bec I can find examples for other param class with no setter: at VmNumaNodeOperationParameters the List vmNumaNodeList as no setter. I can add a setter for the params if you think that will help Line 8: Line 9: private static final long serialVersionUID = 3918909396931144459L; Line 10: Line 11: private Map<Params, Object> params = new HashMap<Params, Object>(); http://gerrit.ovirt.org/#/c/27646/4/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/UpdateVmPolicyVDSParams.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/UpdateVmPolicyVDSParams.java: Line 5: public class UpdateVmPolicyVDSParams extends VdsAndVmIDVDSParametersBase { Line 6: Line 7: private int cpuLimit; Line 8: Line 9: private UpdateVmPolicyVDSParams() { > why having this private constructor? for some some reason I cannot build without a default ctor, and I can see every class that extends from VdsAndVmIDVDSParametersBase creates a default ctor. I don't see any reason to use this ctor so I marked it as private Line 10: } Line 11: Line 12: public UpdateVmPolicyVDSParams(Guid vdsId, Guid vmId, int cpuLimit) { Line 13: super(vdsId, vmId); http://gerrit.ovirt.org/#/c/27646/4/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties: Line 339: VAR__ACTION__DESTROY_DOMAIN=$action destroy Line 340: VAR__ACTION__HOT_PLUG=$action hot plug Line 341: VAR__ACTION__HOT_UNPLUG=$action hot unplug Line 342: VAR__ACTION__HOT_SET_CPUS=$action hot set cpus Line 343: VAR__ACTION__UPDATE_VM_SLA_POLICY=$action update vm sla policy > why not separating the entity and the action as it is for other commands? Done Line 344: VAR__ACTION__LOGON=$action log on Line 345: VAR__ACTION__LOGOFF=$action log off Line 346: VAR__ACTION__ASSIGN=$action assign Line 347: VAR__ACTION__REBALANCE_START=$action rebalance http://gerrit.ovirt.org/#/c/27646/4/packaging/dbscripts/upgrade/03_05_0711_add_vm_sla_policy_version_support.sql File packaging/dbscripts/upgrade/03_05_0711_add_vm_sla_policy_version_support.sql: Line 1: select fn_db_add_config_value('VmSlaPolicySupported','false','3.0'); Line 2: select fn_db_add_config_value('VmSlaPolicySupported','false','3.1'); Line 3: select fn_db_add_config_value('VmSlaPolicySupported','false','3.2'); Line 4: select fn_db_add_config_value('VmSlaPolicySupported','false','3.3'); Line 5: select fn_db_add_config_value('VmSlaPolicySupported','false','3.4'); > no need for a separate upgrade script, please put it in 0000_config.sql Done -- To view, visit http://gerrit.ovirt.org/27646 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I26b0794c52cfe1d352f26392a835023afcd9efa0 Gerrit-PatchSet: 4 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Kobi Ianko <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Jiří Moskovčák <[email protected]> Gerrit-Reviewer: Kobi Ianko <[email protected]> Gerrit-Reviewer: Martin Sivák <[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
