Michael Kublin has posted comments on this change.
Change subject: core: Quota refactor - parameters
......................................................................
Patch Set 3: I would prefer that you didn't submit this
(10 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java
Line 561: addInternalStorageParameters(consumptionParameters);
Line 562: addInternalVdsParameters(consumptionParameters);
Line 563: break;
Line 564: }
Line 565: } catch (ClassCastException e) {
I am agree, these is a bug, we should not catch an exception which is points on
bug, it should be fixed
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 565: } catch (ClassCastException e) {
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:
Wrapper is internal variable why you initialize it inside? It will be cleaned
after exit from method
Line 570: QuotaConsumptionParametersWrapper
quotaConsumptionParametersWrapper;
Line 571: if (getStoragePool() != null) {
Line 572: quotaConsumptionParametersWrapper = new
QuotaConsumptionParametersWrapper(
Line 573: this.getStoragePool(),
this.getReturnValue().getCanDoActionMessages(), this);
Line 567: + ". Quota handling was expected - class does not
implement the expected interface");
Line 568: }
Line 569:
Line 570: QuotaConsumptionParametersWrapper
quotaConsumptionParametersWrapper;
Line 571: if (getStoragePool() != null) {
sorry these code is illogical.
First of all you are running internalValidateAndSetQuota() before
canDoAction(), so what is idea for using
getReturnValue().getCanDoActionMessages().
Line 572: quotaConsumptionParametersWrapper = new
QuotaConsumptionParametersWrapper(
Line 573: this.getStoragePool(),
this.getReturnValue().getCanDoActionMessages(), this);
Line 574:
quotaConsumptionParametersWrapper.setParameters(consumptionParameters);
Line 575: } else {
Line 586: } else {
Line 587: return true;
Line 588: }
Line 589: }
Line 590:
Why void and not return, I think to use return is more convenient in java
Line 591: private void
addInternalVdsParameters(List<QuotaConsumptionParameter> consumptionParameters)
{
Line 592: List<QuotaVdsConsumptionParameter> vdsParameters;
Line 593: vdsParameters = ((QuotaVdsDependent)
this).getQuotaVdsConsumptionParameters();
Line 594: if (null == vdsParameters || vdsParameters.isEmpty()) {
Line 597: } else {
Line 598: consumptionParameters.addAll(vdsParameters);
Line 599: }
Line 600: }
Line 601:
return and not vod
Line 602: private void
addInternalStorageParameters(List<QuotaConsumptionParameter>
consumptionParameters) {
Line 603: List<QuotaStorageConsumptionParameter> storageParameters;
Line 604: storageParameters = ((QuotaStorageDependent)
this).getQuotaStorageConsumptionParameters();
Line 605: if (null == storageParameters || storageParameters.isEmpty())
{
Line 601:
Line 602: private void
addInternalStorageParameters(List<QuotaConsumptionParameter>
consumptionParameters) {
Line 603: List<QuotaStorageConsumptionParameter> storageParameters;
Line 604: storageParameters = ((QuotaStorageDependent)
this).getQuotaStorageConsumptionParameters();
Line 605: if (null == storageParameters || storageParameters.isEmpty())
{
too many this. We don't use them, so you don't use them also. Or should be used
at very place or nowhere.
Line 606: log.error("Command: " + this.getClass().getName()
Line 607: + ". Quota handling was expected - returned empty
parameters");
Line 608: } else {
Line 609: consumptionParameters.addAll(storageParameters);
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/InvalidQuotaParametersException.java
Line 3: import org.ovirt.engine.core.compat.ApplicationException;
Line 4:
Line 5: import java.io.Serializable;
Line 6:
Line 7: public class InvalidQuotaParametersException extends
ApplicationException implements Serializable {
Agree, also you don't need implement Serializable, if one of the parents
implements it a son class which is extends it also implements it.
Line 8: private static final long serialVersionUID = -1759699263394287888L;
Line 9:
Line 10: public InvalidQuotaParametersException() {
Line 11: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/QuotaConsumptionParameter.java
Line 36:
Line 37: public void setQuotaAction(QuotaAction quotaAction) {
Line 38: this.quotaAction = quotaAction;
Line 39: }
Line 40:
I think java has a common way for rewrite clone(), and I don't think that a
wheel should be reinvented again
Line 41: public abstract QuotaConsumptionParameter clone();
Line 42:
Line 43: public void revert() {
Line 44: quotaAction = QuotaAction.CONSUME.equals(quotaAction) ?
QuotaAction.RELEASE : QuotaAction.CONSUME;
Line 38: this.quotaAction = quotaAction;
Line 39: }
Line 40:
Line 41: public abstract QuotaConsumptionParameter clone();
Line 42:
Parameter should not contain any logic. These is a reason for its name parameter
Line 43: public void revert() {
Line 44: quotaAction = QuotaAction.CONSUME.equals(quotaAction) ?
QuotaAction.RELEASE : QuotaAction.CONSUME;
Line 45: }
Line 46:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/QuotaConsumptionParametersWrapper.java
Line 98: }
Line 99:
Line 100: /**
Line 101: * Revert the numbers of all requested cpu, memory and storage
Line 102: */
Why parameter performs revert? It is misleading
Line 103: public void revert() {
Line 104: for (QuotaConsumptionParameter parameter :
this.getParameters()) {
Line 105: parameter.revert();
Line 106: }
--
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: 3
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
