Allon Mureinik has posted comments on this change.
Change subject: core: Quota refactor - parameters
......................................................................
Patch Set 11: I would prefer that you didn't submit this
(11 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java
Line 380: }
Line 381: break;
Line 382: case NEW_ENTITY_ID:
Line 383: daoForEntity.remove(snapshotData);
Line 384: break;
please separate these formatting changes to another patch (or remove them
altogether).
This patch isn't short as-is, and wherever you can make it easier to review,
you should.
Line 385: }
Line 386: }
Line 387:
Line 388: cleanUpCompensationData();
Line 528: } else {
Line 529: endWithFailure();
Line 530: }
Line 531:
Line 532: if (!isInternalExecution()) {
can you explain this condition? it sounds counter intuitive.
Line 533: List<QuotaConsumptionParameter> consumptionParameters =
getQuotaConsumptionParameters();
Line 534: if (consumptionParameters != null) {
Line 535: for (QuotaConsumptionParameter parameter :
consumptionParameters) {
Line 536:
getQuotaManager().removeQuotaFromCache(getStoragePool().getId(),
parameter.getQuotaGuid());
Line 541:
Line 542: private List<QuotaConsumptionParameter>
getQuotaConsumptionParameters() {
Line 543: List<QuotaConsumptionParameter> consumptionParameters = new
ArrayList<QuotaConsumptionParameter>();
Line 544:
Line 545: // This a double marking mechanism which was created to
ensure Quota dependencies would not be inherited
I'd shorten these lines a bit (introduce \n) to make it more readable.
Line 546: // by descendants commands. Each Command is both marked by
the QuotaDependency and implements the required
Line 547: // Interfaces (NONE does not implement any of the two
interfaces).
Line 548: // The enum markings prevent Quota dependencies unintentional
inheritance.
Line 549: switch (getActionType().getQuotaDependency()) {
Line 600: try {
Line 601: returnValue =
Line 602: isUserAuthorizedToRunAction() &&
isBackwardsCompatible() && validateInputs() && acquireLock()
Line 603: && canDoAction();
Line 604: // && internalValidateAndSetQuota();
Don't comment out - just remove it.
Line 605: if (returnValue && this instanceof Quotable) {
Line 606: returnValue &= ((Quotable)
this).validateAndSetQuota();
Line 607: }
Line 608: if (!returnValue &&
getReturnValue().getCanDoActionMessages().size() > 0) {
Line 626: return returnValue;
Line 627: }
Line 628:
Line 629: private boolean internalValidateAndSetQuota() {
Line 630: if (isInternalExecution()) {
here too - can you explain?
Line 631: return true;
Line 632: }
Line 633:
Line 634: if (getStoragePool() == null &&
getActionType().getQuotaDependency() != VdcActionType.QuotaDependency.NONE) {
Line 630: if (isInternalExecution()) {
Line 631: return true;
Line 632: }
Line 633:
Line 634: if (getStoragePool() == null &&
getActionType().getQuotaDependency() != VdcActionType.QuotaDependency.NONE) {
Suggestion: I'd wrap "getActionType().getQuotaDependency() !=
VdcActionType.QuotaD
ependency.NONE)" in a helper method, just to make it slightly more readable.
Line 635: log.error("Command: " + this.getClass().getName()
Line 636: + ". Storage pool is not available for quota
calculation. ");
Line 637: return false;
Line 638: }
Line 640: QuotaConsumptionParametersWrapper
quotaConsumptionParametersWrapper = new QuotaConsumptionParametersWrapper(this,
Line 641: getReturnValue().getCanDoActionMessages());
Line 642:
quotaConsumptionParametersWrapper.setParameters(getQuotaConsumptionParameters());
Line 643:
Line 644: return true;
//quotaConsumptionParametersWrapper.getParameters().isEmpty() ||
getQuotaManager().consume(quotaConsumptionParametersWrapper);
here too - don't comment out - just delete the old code.
Line 645: }
Line 646:
Line 647: /**
Line 648: * @return true if all parameters class and its inner members
passed
Line 820:
addCanDoActionMessage(VdcBllMessages.USER_NOT_AUTHORIZED_TO_PERFORM_ACTION);
Line 821: return false;
Line 822: }
Line 823:
Line 824: if
(!VdcActionType.QuotaDependency.NONE.equals(getActionType().getQuotaDependency()))
{
I prefer == for enums, but it's a matter of taste. I don't think we have a
project wide convention (mkublin - please correct me if I'm wrong)
Line 825: addQuotaPermissionSubject(permSubjects);
Line 826: }
Line 827:
Line 828: for (PermissionSubject permSubject : permSubjects) {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/InvalidQuotaParametersException.java
Line 10: super(errorStr);
Line 11: }
Line 12:
Line 13: public InvalidQuotaParametersException(String errorStr, Throwable
cause) {
Line 14: super("InvalidQuotaParametersException: " + errorStr, cause);
I'd just call super(message, cause)
Line 15: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/QuotaConsumptionParameter.java
Line 4: import org.ovirt.engine.core.compat.Guid;
Line 5:
Line 6: public abstract class QuotaConsumptionParameter {
Line 7: private Guid quotaGuid;
Line 8: private Quota quota;
Why do you need both Quota and QuotaGuid, as separate members?
Line 9: private QuotaAction quotaAction = QuotaAction.CONSUME;
Line 10:
Line 11: protected QuotaConsumptionParameter(Guid quotaGuid, Quota quota,
QuotaAction quotaAction) {
Line 12: this.quotaGuid = quotaGuid;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/QuotaStorageConsumptionParameter.java
Line 5:
Line 6: public class QuotaStorageConsumptionParameter extends
QuotaConsumptionParameter {
Line 7:
Line 8: private Guid storageDomainId;
Line 9: private Double requestedStorageGB;
why not use a primitive double?
Line 10:
Line 11: public QuotaStorageConsumptionParameter(Guid quotaGuid,
Line 12: Quota quota,
Line 13: QuotaAction quotaAction,
--
To view, visit http://gerrit.ovirt.org/8775
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebfc85569ba1aa8bd840f7239f83b7f921a4bd8e
Gerrit-PatchSet: 11
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: ofri masad <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Laszlo Hornyak <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Sharad Mishra <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: ofri masad <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
