Michael Kublin has posted comments on this change.
Change subject: core: Quota refactor - QuotaManager
......................................................................
Patch Set 20: I would prefer that you didn't submit this
(6 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/QuotaManager.java
Line 665: }
Line 666: }
Line 667:
Line 668: }
Line 669:
Why its public? It is look like internal
Line 670: public boolean
validateAndSetClusterQuota(QuotaConsumptionParametersWrapper parameters) {
Line 671:
Line 672: Pair<AuditLogType, AuditLogableBase> auditLogPair = new
Pair<AuditLogType, AuditLogableBase>();
Line 673: auditLogPair.setSecond(parameters.getAuditLogable());
Line 674: lock.readLock().lock();
Line 675: try {
Line 676: if
(storagePoolQuotaMap.get(parameters.getStoragePoolId()) == null) {
Line 677: return false;
Line 678: }
Why I need lock on storagePoolQuotaMap.get(parameters.getStoragePoolId()) ?
You are working on params passed from outside.
Line 679: synchronized
(storagePoolQuotaMap.get(parameters.getStoragePoolId())) {
Line 680: for (QuotaConsumptionParameter parameter :
parameters.getParameters()) {
Line 681: QuotaVdsGroupConsumptionParameter
vdsGroupConsumptionParameter;
Line 682: if (parameter.getParameterType() !=
QuotaConsumptionParameter.ParameterType.VDS_GROUP) {
Line 701: parameters.getCanDoActionMessages()
Line 702:
.add(VdcBllMessages.ACTION_TYPE_FAILED_QUOTA_IS_NOT_VALID.toString());
Line 703: return false;
Line 704: }
Line 705:
just return?
Line 706: boolean success =
Line 707:
checkQuotaClusterLimits(parameters.getAuditLogable()
Line 708: .getStoragePool()
Line 709: .getQuotaEnforcementType(),
Line 975:
Line 976: boolean hardEnforcement =
Line 977: QuotaEnforcementTypeEnum.HARD_ENFORCEMENT ==
parameters.getAuditLogable().getStoragePool().getQuotaEnforcementType();
Line 978:
Line 979: // for each parameter - check and complete
These is a loop, inside it has a locks, so couple of threads will make a
competition on the same locks which are deep down. I like to watch
competitions, but not sure that users of the system also. This is a VERY BAD
practice to put locks inside some private method, you understand that the
complexity of code increased in 1000% with out any reason. These code is a bug
factory.
Line 980: for (QuotaConsumptionParameter param :
parameters.getParameters()) {
Line 981: // check that quota id is valid and fetch the quota from
db (or cache). add the quota to the param
Line 982: boolean validQuotaId = checkAndFetchQuota(parameters,
param);
Line 983:
Line 1102: * Get Quota by Id. If in cache - get from cache. else get from
DAO and add to cache.
Line 1103: * Notice!!! This method apply read/write locks.
Line 1104: * @param quotaId - quota id
Line 1105: * @return - found quota. null if not found.
Line 1106: */
You have race between read and write section.
Line 1107: private Quota fetchQuotaFromCache(Guid quotaId) {
Line 1108: Quota quota = null;
Line 1109: lock.readLock().lock();
Line 1110: try {
Line 1119: if (quota == null) {
Line 1120: try {
Line 1121: lock.writeLock().lock();
Line 1122: // cache in direct quota map
Line 1123: quota = getQuotaDAO().getById(quotaId);
write lock should be after query to DB
Line 1124: if (quota != null) {
Line 1125: directQuotaMap.put(quotaId, quota);
Line 1126: // cache in storage-pool->quota map
Line 1127: if
(!storagePoolQuotaMap.containsKey(quota.getStoragePoolId())) {
--
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: 20
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
