Kobi Ianko has posted comments on this change. Change subject: core: New internal command, VmSlaPolicyCommand ......................................................................
Patch Set 5: (4 comments) http://gerrit.ovirt.org/#/c/27646/5/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 33: protected boolean canDoAction() { Line 34: Line 35: setVmId((Guid) (getParameters().getSlaParameter(VmSlaPolicyParameters.Params.VM_ID))); Line 36: setVdsId((Guid) getParameters().getSlaParameter(VmSlaPolicyParameters.Params.VDS_ID)); Line 37: setVdsGroupId(getVds().getVdsGroupId()); > it's better to have the 3 lines above in the constructor + we might get NPE 1. moving to the ctor, done. 2. we will not get NPE on getVds cause of the setVdsId one line before. Line 38: if (!FeatureSupported.vmSlaPolicy(getVdsGroup().getcompatibility_version())) { Line 39: return failCanDoAction(VdcBllMessages.VM_SLA_POLICY_NOT_SUPPORTED); Line 40: } Line 41: Line 90: } Line 91: Line 92: @Override Line 93: protected void setActionMessageParameters() { Line 94: addCanDoActionMessage(VdcBllMessages.VAR__ACTION__VM_SLA_POLICY); > the entity should be VM (VAR__TYPE__VM) Done Line 95: addCanDoActionMessage(VdcBllMessages.VAR__ACTION__UPDATE_VM_SLA_POLICY); Line 96: } http://gerrit.ovirt.org/#/c/27646/5/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 { > what's the advantage you see in having a map over having separate fields? basically some parameter will not get a value on some scenarios in the near future, so a map that contains only the relevant values is nicer. If you know that this could cause problems I would definitely prefer a work code over a pretty code :) just let me know what are your thoughts 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/5/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties: 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__VM_SLA_POLICY=$action vm sla policy Line 344: VAR__ACTION__UPDATE_VM_SLA_POLICY=$action update vm sla policy > should not include "vm", the entity will be VM Done Line 345: VAR__ACTION__LOGON=$action log on Line 346: VAR__ACTION__LOGOFF=$action log off Line 347: VAR__ACTION__ASSIGN=$action assign Line 348: VAR__ACTION__REBALANCE_START=$action rebalance -- 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: 5 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
