Michael Kublin has posted comments on this change.
Change subject: core: Quota refactor - QuotaManager step 2
......................................................................
Patch Set 5: 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/quota/QuotaManager.java
Line 29: public class QuotaManager {
Line 30: private final static QuotaManager INSTANCE = new QuotaManager();
Line 31: private final static ReentrantReadWriteLock lock = new
ReentrantReadWriteLock();
Line 32: private final static Log log =
LogFactory.getLog(QuotaManager.class);
Line 33: private final static DecimalFormat percentageFormatter = new
DecimalFormat("#.##");
Where is located class QuotaManagerAuditLogger? I did not find it.
Line 34: private final static QuotaManagerAuditLogger AUDIT_LOGGER =
QuotaManagerAuditLogger.getInstance();
Line 35: private final ConcurrentHashMap<Guid, Map<Guid, Quota>>
storagePoolQuotaMap =
Line 36: new ConcurrentHashMap<Guid, Map<Guid, Quota>>();
Line 37: private final ConcurrentHashMap<Guid, Quota> directQuotaMap = new
ConcurrentHashMap<Guid, Quota>();
Line 755: // if a valid storage pool cannot be fund - return false
Line 756: if (!checkAndCompleteStoragePool(parameters)) {
Line 757: return false;
Line 758: }
Line 759:
What regards a case of new storage pool works ok?
Line 760: if
(QuotaEnforcementTypeEnum.DISABLED.equals(parameters.getStorage_pool().getQuotaEnforcementType()))
{
Line 761: return true;
Line 762: }
Line 763:
Line 764: boolean hardEnforcement =
Line 765:
QuotaEnforcementTypeEnum.HARD_ENFORCEMENT.equals(parameters.getStorage_pool().getQuotaEnforcementType());
Line 766:
Line 767: // if one of the main objects of the parameters is null
Line 768: if (null == parameters.getCanDoActionMessages()
How these can be? null == parameters.getAuditLogable() ? or these null ==
parameters.getParameters(). null == parameters.getCanDoActionMessages() always
false.
Line 769: || null == parameters.getAuditLogable()
Line 770: || null == parameters.getParameters()){
Line 771: // if in hard enforcement - return false
Line 772: if (hardEnforcement) {
Line 807: }
Line 808:
Line 809: // check that quota id is valid and fetch the quota from db (or
cache). add the quota to the param
Line 810: private boolean
checkAndFetchQuota(QuotaConsumptionParametersWrapper parameters,
QuotaConsumptionParameter param) {
Line 811: if(param.getQuotaGuid() == null ||
Guid.Empty.equals(param.getQuotaGuid())) {
These is a most ugly way of work, you crated wrapper, passed all parameters and
you are passing a list of canDoMessages all over a code in order to fill it.
Such approach is wrong (I know that it is used at our code, but sometimes
someone should do something smart), you should use return value.
Line 812:
parameters.getCanDoActionMessages().add(VdcBllMessages.ACTION_TYPE_FAILED_QUOTA_IS_NO_LONGER_AVAILABLE_IN_SYSTEM.toString());
Line 813: log.errorFormat("No Quota id passed from command:
{0}",parameters.getAuditLogable().getClass().getName());
Line 814: return false;
Line 815: }
Line 831: private boolean
checkAndCompleteStoragePool(QuotaConsumptionParametersWrapper parameters) {
Line 832: // if storage_pool is null
Line 833: if (parameters.getStorage_pool() == null){
Line 834: log.debug("Null storage pool was passed to
'QuotaManager.validateAndSetStorageQuota()'");
Line 835: // if no quota was passed
You add to many checks in order to prevent a NullPointerException, but if it
really can happened? You are creating a parameters, you are not safe about your
code?
Line 836: if (parameters.getParameters().isEmpty() ||
parameters.getParameters().get(0).getQuotaGuid() == null) {
Line 837:
parameters.getCanDoActionMessages().add(VdcBllMessages.ACTION_TYPE_FAILED_QUOTA_IS_NOT_VALID.toString());
Line 838: return false;
Line 839: }
Line 836: if (parameters.getParameters().isEmpty() ||
parameters.getParameters().get(0).getQuotaGuid() == null) {
Line 837:
parameters.getCanDoActionMessages().add(VdcBllMessages.ACTION_TYPE_FAILED_QUOTA_IS_NOT_VALID.toString());
Line 838: return false;
Line 839: }
Line 840: // try to get storage pool from quota
Please explain that case, the pool was not found on parameters passed from
user,
but you found it by quota? How that can be?
Line 841: storage_pool storage_pool =
getStoragePoolByQuota(parameters.getParameters().get(0).getQuotaGuid());
Line 842: if (null != storage_pool) {
Line 843: parameters.setStorage_pool(storage_pool);
Line 844: } else {
Line 854: // vds group id which is handled by this quota
Line 855: private boolean
checkVdsGroupMatchQuota(QuotaConsumptionParametersWrapper parameters,
QuotaConsumptionParameter param) {
Line 856: Quota quota = param.getQuota();
Line 857:
Line 858: // make sure vds group is in the quota
Maybe better perform such check outside a method, will be much easier to
understand and debug
Line 859: if (param instanceof QuotaVdsConsumptionParameter) {
Line 860: QuotaVdsConsumptionParameter paramVds =
(QuotaVdsConsumptionParameter) param;
Line 861:
Line 862: if (paramVds.getVdsGroupId() == null) {
Line 857:
Line 858: // make sure vds group is in the quota
Line 859: if (param instanceof QuotaVdsConsumptionParameter) {
Line 860: QuotaVdsConsumptionParameter paramVds =
(QuotaVdsConsumptionParameter) param;
Line 861:
Again, these should not pass validateInputs()
Line 862: if (paramVds.getVdsGroupId() == null) {
Line 863:
parameters.getCanDoActionMessages().add(VdcBllMessages.ACTION_TYPE_FAILED_QUOTA_IS_NOT_VALID.toString());
Line 864: log.errorFormat("Quota Vds parameters from command:
{0} are missing vds group id",
Line 865:
parameters.getAuditLogable().getClass().getName());
Line 894:
Line 895: // make sure vds group is in the quota
Line 896: if (param instanceof QuotaStorageConsumptionParameter) {
Line 897: QuotaStorageConsumptionParameter paramstorage =
(QuotaStorageConsumptionParameter) param;
Line 898:
Same here, how passed canDoaACtion().validateInputs() ?
Line 899: if (paramstorage.getStorageDomainId() == null) {
Line 900:
parameters.getCanDoActionMessages().add(VdcBllMessages.ACTION_TYPE_FAILED_QUOTA_IS_NOT_VALID.toString());
Line 901: log.errorFormat("Quota storage parameters from
command: {0} are missing storage domain id",
Line 902:
parameters.getAuditLogable().getClass().getName());
Line 936: // cache in direct quota map
Line 937: Quota quota = getQuotaDAO().getById(quotaId);
Line 938: directQuotaMap.put(quotaId, quota);
Line 939: // cache in storage-pool->quota map
Line 940:
storagePoolQuotaMap.putIfAbsent(quota.getStoragePoolId(), new HashMap<Guid,
Quota>());
Possible NullPointerException
Line 941:
storagePoolQuotaMap.get(quota.getStoragePoolId()).put(quotaId, quota);
Line 942: }
Line 943: return directQuotaMap.get(quotaId);
Line 944: }
--
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: 5
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
