Liron Aravot has posted comments on this change.
Change subject: core: Quota refactor - QuotaManager step 2
......................................................................
Patch Set 9: (9 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/QuotaManager.java
Line 772: boolean validQuotaId = checkAndFetchQuota(parameters,
param);
Line 773:
Line 774: // In case this param is a QuotaVdsConsumptionParameter -
check that it has a valid
Line 775: // vds group id which is handled by this quota
Line 776: boolean validVdsGroup = false;
why not set it to true during creation and avoid the else clause?
otherwise you don't need to explicitly set false to it during creation.
Line 777: if (param instanceof QuotaVdsConsumptionParameter) {
Line 778: validVdsGroup = checkVdsGroupMatchQuota(parameters,
param);
Line 779: } else {
Line 780: validVdsGroup = true;
Line 780: validVdsGroup = true;
Line 781: }
Line 782:
Line 783: // In case this param is a
QuotaStorageConsumptionParameter - check that it has a valid
Line 784: // storage domain id which is handled by this quota
why not set it to true during creation and avoid the else clause?
otherwise you don't need to explicitly set false to it during creation.
Line 785: boolean validStorageDomain = false;
Line 786: if (param instanceof QuotaStorageConsumptionParameter) {
Line 787: validStorageDomain =
checkStoragePoolMatchQuota(parameters, param);
Line 788: } else {
Line 812: return false;
Line 813: }
Line 814:
Line 815: Quota quota = fetchQuotaFromDB(param.getQuotaGuid());
Line 816: if (quota == null) {
you can use failCanDoAction method instead.
Line 817:
parameters.getCanDoActionMessages().add(VdcBllMessages.ACTION_TYPE_FAILED_QUOTA_IS_NO_LONGER_AVAILABLE_IN_SYSTEM.toString());
Line 818: log.errorFormat("The quota id {0} is not found in backend
and DB.", param.getQuotaGuid().toString());
Line 819: return false;
Line 820: } else {
Line 819: return false;
Line 820: } else {
Line 821: param.setQuota(quota);
Line 822: }
Line 823: if
(!quota.getStoragePoolId().equals(parameters.getStoragePoolGuid())) {
can you actually reach this state? why do you throw an exception that is such
an expensive operation?
Line 824: throw new InvalidQuotaParametersException("The Quota
storage pool id does not match the passed storage pool");
Line 825: }
Line 826: return true;
Line 827: }
Line 831: private boolean
checkVdsGroupMatchQuota(QuotaConsumptionParametersWrapper parameters,
QuotaConsumptionParameter param) {
Line 832: Quota quota = param.getQuota();
Line 833: QuotaVdsConsumptionParameter paramVds =
(QuotaVdsConsumptionParameter) param;
Line 834:
Line 835: if (paramVds.getVdsGroupId() == null) {
failCanDoAction instead would be better.
Line 836:
parameters.getCanDoActionMessages().add(VdcBllMessages.ACTION_TYPE_FAILED_QUOTA_IS_NOT_VALID.toString());
Line 837: log.errorFormat("Quota Vds parameters from command: {0}
are missing vds group id",
Line 838:
parameters.getAuditLogable().getClass().getName());
Line 839: return false;
Line 837: log.errorFormat("Quota Vds parameters from command: {0}
are missing vds group id",
Line 838:
parameters.getAuditLogable().getClass().getName());
Line 839: return false;
Line 840: }
Line 841: boolean vdsGroupInQuota = false;
why not create as true and avoid the first if clause?
Line 842: if(quota.getGlobalQuotaVdsGroup() != null) {
Line 843: vdsGroupInQuota = true;
Line 844: } else {
Line 845: for (QuotaVdsGroup vdsGroup : quota.getQuotaVdsGroups()) {
Line 870: log.errorFormat("Quota storage parameters from command:
{0} are missing storage domain id",
Line 871:
parameters.getAuditLogable().getClass().getName());
Line 872: return false;
Line 873: }
Line 874: boolean storageDomainInQuota = false;
why not create is as true and avoid the first if clause?
Line 875: if(quota.getGlobalQuotaStorage() != null) {
Line 876: storageDomainInQuota = true;
Line 877: } else {
Line 878: for (QuotaStorage quotaStorage :
quota.getQuotaStorages()) {
Line 882: }
Line 883: }
Line 884: }
Line 885:
Line 886: if (!storageDomainInQuota) {
failCanDoAction
Line 887:
parameters.getCanDoActionMessages().add(VdcBllMessages.ACTION_TYPE_FAILED_QUOTA_IS_NOT_VALID.toString());
Line 888: log.errorFormat("Quota storage parameters from command:
{0}. Storage domain does not match quota",
Line 889:
parameters.getAuditLogable().getClass().getName());
Line 890: return false;
Line 902: if (quotaId != null && quotaId != Guid.Empty) {
Line 903: if (!directQuotaMap.contains(quotaId)) {
Line 904: // cache in direct quota map
Line 905: Quota quota = getQuotaDAO().getById(quotaId);
Line 906: lock.readLock().lock();
in addition to yair commend that should be addressed, i'd put that lock in
try/finally to avoid any possible problems that can be hazardous.
Line 907:
Line 908: directQuotaMap.put(quotaId, quota);
Line 909: // cache in storage-pool->quota map
Line 910:
storagePoolQuotaMap.putIfAbsent(quota.getStoragePoolId(), new HashMap<Guid,
Quota>());
--
To view, visit http://gerrit.ovirt.org/8777
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I15314d6269d1f5f43de17c36a6ae91ba874c8a58
Gerrit-PatchSet: 9
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: Liron Aravot <[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
