Allon Mureinik has posted comments on this change.
Change subject: core: Quota refactor - parameters
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(20 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java
Line 539: return returnValue;
Line 540: }
Line 541:
Line 542: private boolean internalValidateAndSetQuota() {
Line 543: if (isInternalExecution()) {
What the reasoning behind this approach?
Consider the following:
Creating a snapshot iterates over all the VM's disks and fires an internal
command to snapshot the disk.
Would you want the quota calculation at the lowest building block possible?
Line 544: return true;
Line 545: }
Line 546:
Line 547: VdcActionType.QuotaDependency quotaDependency =
this.getActionType().getQuotaDependency();
Line 543: if (isInternalExecution()) {
Line 544: return true;
Line 545: }
Line 546:
Line 547: VdcActionType.QuotaDependency quotaDependency =
this.getActionType().getQuotaDependency();
"this." is redundant.
Line 548: List<QuotaConsumptionParameter> consumptionParameters = new
ArrayList<QuotaConsumptionParameter>();
Line 549:
Line 550: try {
Line 551: switch (quotaDependency) {
Line 547: VdcActionType.QuotaDependency quotaDependency =
this.getActionType().getQuotaDependency();
Line 548: List<QuotaConsumptionParameter> consumptionParameters = new
ArrayList<QuotaConsumptionParameter>();
Line 549:
Line 550: try {
Line 551: switch (quotaDependency) {
I'm toying with the idea of suggesting a bit-mask instead of this switch case.
Still not sure what I think about it.
Line 552: case NONE:
Line 553: return true;
Line 554: case STORAGE:
Line 555: addInternalStorageParameters(consumptionParameters);
Line 561: addInternalStorageParameters(consumptionParameters);
Line 562: addInternalVdsParameters(consumptionParameters);
Line 563: break;
Line 564: }
Line 565: } catch (ClassCastException e) {
Can't you wrap this with a checked exception?
Line 566: log.error("Command: " + this.getClass().getName()
Line 567: + ". Quota handling was expected - class does not
implement the expected interface");
Line 568: }
Line 569:
Line 569:
Line 570: QuotaConsumptionParameters quotaConsumptionParameters;
Line 571: if (getStoragePool() != null) {
Line 572: quotaConsumptionParameters = new
QuotaConsumptionParameters(
Line 573: this.getStoragePool(),
this.getStoragePool().getId(),
the storagePool and storagePoolId are duplicate information - see comment in
the relevant class.
also, the calls to "this." are redundant.
Line 574: this.getReturnValue().getCanDoActionMessages(),
this);
Line 575:
quotaConsumptionParameters.setParameters(consumptionParameters);
Line 576: } else {
Line 577: quotaConsumptionParameters = new
QuotaConsumptionParameters(
Line 570: QuotaConsumptionParameters quotaConsumptionParameters;
Line 571: if (getStoragePool() != null) {
Line 572: quotaConsumptionParameters = new
QuotaConsumptionParameters(
Line 573: this.getStoragePool(),
this.getStoragePool().getId(),
Line 574: this.getReturnValue().getCanDoActionMessages(),
this);
In a similar fashion, passing the command and the canDoMessages in redundant -
the second can be infered from the first.
Line 575:
quotaConsumptionParameters.setParameters(consumptionParameters);
Line 576: } else {
Line 577: quotaConsumptionParameters = new
QuotaConsumptionParameters(
Line 578: null, null,
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/InvalidQuotaParametersException.java
Line 1: package org.ovirt.engine.core.bll.quota;
Line 2:
Line 3: import org.ovirt.engine.core.compat.ApplicationException;
Line 4:
Line 5: public class InvalidQuotaParametersException extends
ApplicationException implements java.io.Serializable {
replace FQCN with import.
Line 6: private static final long serialVersionUID = -1759699263394287888L;
Line 7:
Line 8: public InvalidQuotaParametersException() {
Line 9: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/QuotaConsumptionParameter.java
Line 43: public void revert() {
Line 44: this.action = Action.CONSUME.equals(action) ? Action.RELEASE :
Action.CONSUME;
Line 45: }
Line 46:
Line 47: public enum Action {
Action is a bit too general.
How about renaming it to QuotaAction?
Line 48: CONSUME, RELEASE
Line 49: }
....................................................
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 {
throughout the class - stop using "this."
Line 11:
Line 12: private List<QuotaConsumptionParameter> parameters;
Line 13:
Line 14: private org.ovirt.engine.core.common.businessentities.storage_pool
storage_pool;
Line 10: public class QuotaConsumptionParameters {
Line 11:
Line 12: private List<QuotaConsumptionParameter> parameters;
Line 13:
Line 14: private org.ovirt.engine.core.common.businessentities.storage_pool
storage_pool;
Replace FQCN with import.
Line 15: private Guid storagePoolGuid;
Line 16: private ArrayList<String> canDoActionMessages;
Line 17: private AuditLogableBase auditLogable;
Line 18:
Line 16: private ArrayList<String> canDoActionMessages;
Line 17: private AuditLogableBase auditLogable;
Line 18:
Line 19: public QuotaConsumptionParameters(storage_pool storage_pool,
Line 20: Guid storagePoolGuid,
This is redundant - if you have the storage_pool, you also have the
storage_pool_id - don't hold two members.
If you want an easy getter, just implement
Gudi getStoragePoolId() {
return storage_pool.getId()
}
Line 21: ArrayList<String> canDoActionMessages,
Line 22: AuditLogableBase auditLogable) {
Line 23: this.storage_pool = storage_pool;
Line 24: this.storagePoolGuid = storagePoolGuid;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/QuotaStorageConsumptionParameter.java
Line 2:
Line 3: import org.ovirt.engine.core.common.businessentities.Quota;
Line 4: import org.ovirt.engine.core.compat.Guid;
Line 5:
Line 6: public class QuotaStorageConsumptionParameter extends
QuotaConsumptionParameter {
throughout the class - stop using "this."
Line 7:
Line 8: private Guid storageDomainId;
Line 9: private long requestedStorageGB;
Line 10:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/QuotaStorageDependent.java
Line 4: import java.util.List;
Line 5:
Line 6: /**
Line 7: * Implement the QuotaStorageDependent interface to identify your
command as one that dependent on
Line 8: * Storage Quota calculation in order to run. If a Command handles
disks, images, snapshots and so on -
s/a Command/the command/
Line 9: * it should be QuotaStorageDependent.
Line 10: */
Line 11: public interface QuotaStorageDependent {
Line 12:
Line 15: * Override this method in order to set the storage consumption
parameters for the quota check.
Line 16: * This method is called by CommandBase during the canDoAction
check in order to make sure the
Line 17: * command has sufficient quota resources in order to run.
Line 18: *
Line 19: * return null if the command does consume any storage resources.
what is the semantic of returning null?
In such a case, wouldn't I just make my command not implement this interface?
Line 20: *
Line 21: * @return - list of storage consumption parameters. null if no
consumption.
Line 22: */
Line 23: public List<QuotaStorageConsumptionParameter>
getQuotaStorageConsumptionParameters();
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/QuotaVdsConsumptionParameter.java
Line 2:
Line 3: import org.ovirt.engine.core.common.businessentities.Quota;
Line 4: import org.ovirt.engine.core.compat.Guid;
Line 5:
Line 6: public class QuotaVdsConsumptionParameter extends
QuotaConsumptionParameter {
throughout the class- sop using "this."
Line 7:
Line 8: private Guid vdsGroupId;
Line 9: private int requestedCpu;
Line 10: private long requestedMemory;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/QuotaVdsDependent.java
Line 4: import java.util.List;
Line 5:
Line 6: /**
Line 7: * Implement the QuotaVdsDependent interface to identify your command
as one that dependent on
Line 8: * Vds(vcpu and/or memory) Quota calculation in order to run. If a
Command handles vcpus and memory -
missing space after Vds.
Line 9: * it should be QuotaVdsDependent.
Line 10: */
Line 11: public interface QuotaVdsDependent {
Line 12:
Line 15: * Override this method in order to set the vds consumption
parameters for the quota check.
Line 16: * This method is called by CommandBase during the canDoAction
check in order to make sure the
Line 17: * command has sufficient quota resources in order to run.
Line 18: *
Line 19: * return null if the command does consume any vds resources.
s/does/doen't/
Line 20: *
Line 21: * @return - list of vds consumption parameters. null if no
consumption.
Line 22: */
Line 23: public List<QuotaVdsConsumptionParameter>
getQuotaVdsConsumptionParameters();
Line 19: * return null if the command does consume any vds resources.
Line 20: *
Line 21: * @return - list of vds consumption parameters. null if no
consumption.
Line 22: */
Line 23: public List<QuotaVdsConsumptionParameter>
getQuotaVdsConsumptionParameters();
here too - what are the semantics of returning null?
Wouldn't I rather just not implement this interface?
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java
Line 323: return mappings.get(value);
Line 324: }
Line 325:
Line 326: public QuotaDependency getQuotaDependency() {
Line 327: return this.quotaDependency;
s/this.//
Line 328: }
Line 329:
Line 330: public enum QuotaDependency {
Line 331: NONE, STORAGE, VDS, BOTH
....................................................
Commit Message
Line 5: CommitDate: 2012-10-24 17:04:26 +0200
Line 6:
Line 7: core: Quota refactor - parameters
Line 8:
Line 9: First step to Quota refactor.
Having a wiki or something like just describing where you are /trying/ to get
to will be very helpful.
Stating "first step" here is a bit out of context.
Line 10:
Line 11: Adding Objects for Quota consumption parameters
Line 12: Adding InvalidQuotaParametersException
Line 13:
--
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: 2
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
