Gilad Chaplik has posted comments on this change.
Change subject: core: Quota refactor - parameters
......................................................................
Patch Set 1: (14 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java
Line 543: if (isInternalExecution()) {
Line 544: return true;
Line 545: }
Line 546:
Line 547: VdcActionType.QuotaDependency quotaDependency =
this.getActionType().getQuotaDependency();
consider moving QuotaDependency to a separate file
Line 548: List<QuotaConsumptionParameter> consumptionParameters = new
ArrayList<QuotaConsumptionParameter>();
Line 549: QuotaConsumptionParameters quotaConsumptionParameters = new
QuotaConsumptionParameters(
Line 550: this.getStoragePool(), this.getStoragePool().getId(),
Line 551: this.getReturnValue().getCanDoActionMessages(), this);
Line 546:
Line 547: VdcActionType.QuotaDependency quotaDependency =
this.getActionType().getQuotaDependency();
Line 548: List<QuotaConsumptionParameter> consumptionParameters = new
ArrayList<QuotaConsumptionParameter>();
Line 549: QuotaConsumptionParameters quotaConsumptionParameters = new
QuotaConsumptionParameters(
Line 550: this.getStoragePool(), this.getStoragePool().getId(),
please verify that storagePool != null;
and create these params only if QuotaDependency != none;
no need for 'this.getStoragePool()' getStoragePool() is enough.
Line 551: this.getReturnValue().getCanDoActionMessages(), this);
Line 552:
Line 553: try {
Line 554: switch (quotaDependency) {
Line 561: getInternalVdsParameters(consumptionParameters);
Line 562: break;
Line 563: case BOTH:
Line 564: getInternalStorageParameters(consumptionParameters);
Line 565: getInternalStorageParameters(consumptionParameters);
vdsGroup?
Line 566: break;
Line 567: }
Line 568: } catch (ClassCastException e) {
Line 569: log.error("Command: " + this.getClass().getName()
Line 586: + ". Quota handling was expected - returned empty
parameters");
Line 587: } else {
Line 588: consumptionParameters.addAll(vdsParameters);
Line 589: }
Line 590: }
consider having one getParameters method for both.
[must] change method name to include the word quota!
Line 591:
Line 592: private void
getInternalStorageParameters(List<QuotaConsumptionParameter>
consumptionParameters) {
Line 593: List<QuotaStorageConsumptionParameter> storageParameters;
Line 594: storageParameters = ((QuotaStorageDependent)
this).getQuotaStorageConsumptionParameters();
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/QuotaConsumptionParameters.java
Line 6:
Line 7: import java.util.ArrayList;
Line 8: import java.util.List;
Line 9:
Line 10: public class QuotaConsumptionParameters {
don't know why we need a wrapper here; we should go for one class that holds
the list.
Line 11:
Line 12: private List<QuotaConsumptionParameter> parameters;
Line 13:
Line 14: private QuotaConsumptionParametersMetaData metaData;
Line 77:
Line 78: public void setAuditLogable(AuditLogableBase auditLogable) {
Line 79: this.metaData.setAuditLogable(auditLogable);
Line 80: }
Line 81:
don't see a reason to clone a parameter class...
Line 82: public QuotaConsumptionParameters clone() {
Line 83: QuotaConsumptionParameters clone = new
QuotaConsumptionParameters(
Line 84: this.getStorage_pool(),
Line 85: this.getStoragePoolGuid(),
Line 109:
Line 110: public void setMetaData(QuotaConsumptionParametersMetaData
metaData) {
Line 111: this.metaData = metaData;
Line 112: }
Line 113:
why we need transactionId?
Line 114: public Guid getTransactionId() {
Line 115: return this.metaData.getTransactionID();
Line 116: }
Line 117:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/QuotaConsumptionParametersMetaData.java
Line 62: this.canDoActionMessages,
Line 63: this.auditLogable);
Line 64: }
Line 65:
Line 66: public Guid getTransactionID() {
pls. explain this member
Line 67: return transactionID;
Line 68: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/QuotaStorageConsumptionParameter.java
Line 16: this.quotaGuid = quotaGuid;
Line 17: this.quota = quota;
Line 18: this.action = action;
Line 19: this.storageDomainId = storageDomainId;
Line 20: this.requestedStorageGB = requestedStorageGB;
?
Line 21: }
Line 22:
Line 23: public Guid getStorageDomainId() {
Line 24: return storageDomainId;
Line 42: this.quotaGuid,
Line 43: this.quota,
Line 44: this.action,
Line 45: this.storageDomainId,
Line 46: this.requestedStorageGB);
no need for 'this'.
Line 47: }
Line 48:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/QuotaVdsConsumptionParameter.java
Line 19: this.quota = quota;
Line 20: this.action = action;
Line 21: this.vdsGroupId = vdsGroupId;
Line 22: this.requestedCpu = requestedCpu;
Line 23: this.requestedMemory = requestedMemory;
?
Line 24: }
Line 25:
Line 26: public Guid getVdsGroupId() {
Line 27: return vdsGroupId;
Line 54: this.quota,
Line 55: this.action,
Line 56: this.vdsGroupId,
Line 57: this.requestedCpu,
Line 58: this.requestedMemory);
no need for 'this', use getters for super private members
Line 59: }
Line 60:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java
Line 270: }
Line 271: }
Line 272:
Line 273: private VdcActionType(int value) {
Line 274: this(value, (ActionGroup) null);
null casting, please provide comment or try to change it
Line 275: }
Line 276:
Line 277: private VdcActionType(int value , QuotaDependency
quotaDependency) {
Line 278: this(value, null, quotaDependency);
Line 323: return mappings.get(value);
Line 324: }
Line 325:
Line 326: public QuotaDependency getQuotaDependency() {
Line 327: return this.quotaDependency;
this?
Line 328: }
Line 329:
Line 330: public enum QuotaDependency {
Line 331: NONE, STORAGE, VDS, BOTH
--
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: 1
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]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
