Michael Kublin has posted comments on this change.
Change subject: core: Quota refactor - QuotaManager
......................................................................
Patch Set 22: I would prefer that you didn't submit this
(7 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/QuotaManager.java
Line 115:
Line 116: private boolean validateAndSetStorageQuotaHelper(storage_pool
storagePool,
Line 117: List<StorageQuotaValidationParameter> parameters,
Line 118: ArrayList<String> canDoActionMessages, boolean
isIncrease) {
Line 119: Pair<AuditLogType, AuditLogableBase> logPair = new
Pair<AuditLogType, AuditLogableBase>();
The lock is read
Line 120: lock.readLock().lock();
Line 121: try {
Line 122: if (storagePool == null) {
Line 123: log.debug("Null storage pool was passed to
'QuotaManager.validateAndSetStorageQuota()'");
Line 142: return true;
Line 143: }
Line 144: }
Line 145: }
Line 146:
Internal cache changed, lock should be write
Line 147: if
(!storagePoolQuotaMap.containsKey(storagePool.getId())) {
Line 148: storagePoolQuotaMap.put(storagePool.getId(), new
HashMap<Guid, Quota>());
Line 149: }
Line 150:
Line 447: }
Line 448:
Line 449: /**
Line 450: * This method is protected for testing use only
Line 451: */
These method still in use?
Line 452: protected void auditLog(Pair<AuditLogType, AuditLogableBase>
logPair) {
Line 453: if (logPair.getFirst() != null) {
Line 454: AuditLogDirector.log(logPair.getSecond(),
logPair.getFirst());
Line 455: }
Line 728: }
Line 729: if (QuotaEnforcementTypeEnum.DISABLED ==
storagePool.getQuotaEnforcementType()) {
Line 730: return true;
Line 731: }
Line 732: if
(!storagePoolQuotaMap.containsKey(storagePool.getId())) {
Same here lock is read, but you changing a shared state.
Line 733: storagePoolQuotaMap.put(storagePool.getId(), new
HashMap<Guid, Quota>());
Line 734: }
Line 735:
Line 736: synchronized
(storagePoolQuotaMap.get(storagePool.getId())) {
Line 894: try {
Line 895: storage_pool storagePool =
parameters.getAuditLogable().getStoragePool();
Line 896: if (storagePool == null) {
Line 897: throw new InvalidQuotaParametersException("Null
storage pool passed to QuotaManager");
Line 898: }
Same here, read lock
Line 899: if
(!storagePoolQuotaMap.containsKey(storagePool.getId())) {
Line 900: storagePoolQuotaMap.put(storagePool.getId(), new
HashMap<Guid, Quota>());
Line 901: }
Line 902: synchronized
(storagePoolQuotaMap.get(storagePool.getId())) {
Line 899: if
(!storagePoolQuotaMap.containsKey(storagePool.getId())) {
Line 900: storagePoolQuotaMap.put(storagePool.getId(), new
HashMap<Guid, Quota>());
Line 901: }
Line 902: synchronized
(storagePoolQuotaMap.get(storagePool.getId())) {
Line 903: if (!validateAndCompleteParameters(parameters,
auditLogPair)) {
The following exception also is thrown for some down message, ass throws to
method signature. Al least at private methods
Line 904: throw new InvalidQuotaParametersException();
Line 905: }
Line 906: return internalConsumeAndReleaseHandler(parameters,
auditLogPair);
Line 907: }
Line 1118: // if quota was not found in cache - look for it in DB
Line 1119: if (quota == null) {
Line 1120: quota = getQuotaDAO().getById(quotaId);
Line 1121: if (quota != null) {
Line 1122: // cache in quota map
How these can be? That I get from DB a quota which is not related to pool and
why it catch only here?. If method throws exception, throws should be added to
its signature
Line 1123: if (storagePoolId.equals(quota.getStoragePoolId())) {
Line 1124: quotaMap.put(quotaId, quota);
Line 1125: } else {
Line 1126: throw new InvalidQuotaParametersException(
--
To view, visit http://gerrit.ovirt.org/8776
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb100467a55b26e4219d1a2562da86b81ffdc032
Gerrit-PatchSet: 22
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
