Allon Mureinik has posted comments on this change.
Change subject: core: refactoring quota
......................................................................
Patch Set 3: Fails; I would prefer that you didn't submit this
(16 inline comments)
See comments inline.
Also, this patch is incomplete: you move QuotaHelper from
org.ovirt.engine.core.bll.QuotaHelper to
org.ovirt.engine.core.bll.quota.QuotaHelper, but you did not fix the references
in other commands - this will break the build.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java
Line 308: ((Quotable) this).rollbackQuota();
Not a fan of this run-time type checking.
Although I'm guessing changing the command hierarchy at his stage will be
somewhat of a pain.
Line 498: returnValue &= ((Quotable)
this).validateAndSetQuota();
Your canDoAction modifies the quota?
this sounds a bit counter intuitive.
Line 514
+gazilion!
Line 1129: ((Quotable) this).rollbackQuota();
You're implying that for quota handling there is no difference between rolling
back and compensating.
Not sure this is the desired behavior.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/QuotaHelper.java
Line 401: return Config.<Integer>
GetValue(ConfigValues.QuotaThresholdVdsGroup);
I'm not strongly opposed to this change - but please explain why it was done,
and what's your benefit here.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/QuotaManager.java
Line 26: public class QuotaManager {
you moved this file instead of "git mv" it - which lost all it's history.
please use the proper way of moving.
Line 37: .getQuotaDAO();
why not in one line?
Line 57: private static Log log = LogFactory.getLog(QuotaManager.class);
Make it final, and move it up together with the other constants.
Line 60: new ConcurrentHashMap<Guid, Map<Guid, Quota>>();
here too.
Line 79: synchronized
(storagePoolQuotaMap.get(storagePool.getId())) {
there is a race condition here.
You can decide the map contains this ID, then remove it from another thread and
only then enter the synchronized block that assumes it exists.
Double checking should solve this.
Also - why use two types of locking?
If you already have a ReadWriteLock,why not use the write lock instead of
synchronizing?
Line 98: if (!validateAndSetStorageQuotaHelper(storagePool,
parameters, new ArrayList<String>(), false)) {
same here.
Line 126: // don't rollback if the storage pool is not in cache
(it's already persist)
s/persist/persisted/
Line 344: // don't rollback if the storage pool is not in cache
(it's already persist)
s/persist/persisted/
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/StorageQuotaValidationParameter.java
Line 11: super();
super() is redundant.
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/QuotaHelperTest.java
Line 1
Why delete this file, for the lock of g-d?
With such a refactor you should also refactor the test, not just discard it and
hope fot the best.
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/Quotable.java
Line 5: public interface Quotable {
Quotable is something that can be quoted - and has nothing to do with quota.
How about QuotaEnabled?
--
To view, visit http://gerrit.ovirt.org/6301
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Idc7573082e777370cdf0b88dbe5bfedeb5d02baf
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Laszlo Hornyak <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
