ofri masad has posted comments on this change.
Change subject: core: Add mom policy update command
......................................................................
Patch Set 5: (7 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateMomPolicyCommand.java
Line 27: public UpdateMomPolicyCommand(VdsActionParameters
vdsActionParameters) {
Line 28: super(vdsActionParameters);
Line 29: }
Line 30:
Line 31: private boolean runUpdateMomPolicy(final VDS vds, boolean
ballooningEnabled) {
Done
Line 32: boolean succeeded = false;
Line 33: try {
Line 34: succeeded =
runVdsCommand(VDSCommandType.SetMOMPolicyParameters,
Line 35: new MomPolicyVDSParameters(vds,
ballooningEnabled)).getSucceeded();
Line 48: protected boolean canDoAction() {
Line 49: VdsValidator vdsValidator = new VdsValidator(getVds());
Line 50:
Line 51: ValidationResult exists = vdsValidator.exists();
Line 52: if (!exists.isValid()) {
Done
Line 53: return failCanDoAction(exists.getMessage());
Line 54: }
Line 55:
Line 56: ValidationResult up =
vdsValidator.validateStatus(VDSStatus.Up);
Line 68: }
Line 69:
Line 70: @Override
Line 71: public AuditLogType getAuditLogTypeValue() {
Line 72: return getSucceeded() ?
Done
Line 73: AuditLogType.USER_UPDATED_MOM_POLICIES :
Line 74: AuditLogType.USER_FAILED_TO_UPDATE_MOM_POLICIES;
Line 75: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VdsValidator.java
Line 83: }
Line 84:
Line 85: public ValidationResult validateStatus(VDSStatus vdsStatus) {
Line 86: ValidationResult existsValidation = exists();
Line 87: if (!existsValidation.isValid()) {
why allowing NPE if you can avoid it in zero cost? if someone uses this code,
he presumes it is null safe. I don't see a reason throw an exception when can
easily return a specific error message.
Line 88: return existsValidation;
Line 89: }
Line 90: if (vdsStatus != vds.getStatus()) {
Line 91: return new
ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_VDS_STATUS_ILLEGAL);
Line 86: ValidationResult existsValidation = exists();
Line 87: if (!existsValidation.isValid()) {
Line 88: return existsValidation;
Line 89: }
Line 90: if (vdsStatus != vds.getStatus()) {
VDSStatus is enum. although I usually agree with your approach, I got many
reviews telling me that this is project convention to use ==/!= for enums.
Line 91: return new
ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_VDS_STATUS_ILLEGAL);
Line 92: }
Line 93: return ValidationResult.VALID;
Line 94: }
Line 87: if (!existsValidation.isValid()) {
Line 88: return existsValidation;
Line 89: }
Line 90: if (vdsStatus != vds.getStatus()) {
Line 91: return new
ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_VDS_STATUS_ILLEGAL);
Done
Line 92: }
Line 93: return ValidationResult.VALID;
Line 94: }
Line 89: }
Line 90: if (vdsStatus != vds.getStatus()) {
Line 91: return new
ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_VDS_STATUS_ILLEGAL);
Line 92: }
Line 93: return ValidationResult.VALID;
Done
Line 94: }
--
To view, visit http://gerrit.ovirt.org/17379
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I64350c17f182c1dd0ba1d06130e5d56019b32fc9
Gerrit-PatchSet: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: ofri masad <oma...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com>
Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com>
Gerrit-Reviewer: Martin Sivák <msi...@redhat.com>
Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com>
Gerrit-Reviewer: Roy Golan <rgo...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-Reviewer: ofri masad <oma...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
