Gilad Chaplik has posted comments on this change.
Change subject: core: Quota Monitors - backend
......................................................................
Patch Set 3: I would prefer that you didn't submit this
(12 inline comments)
I will tell you one thing, the query should contain a big comment that the it
selects usage only for selected VMs, and not in resource tab (all VMs). if this
is not the case, I'm sorry but I don't agree with this solution, the UI will
not fetch all user's VM in order to send a list of ids back to the backend,
luckily I'm now reviewing the next UI patches, so for now it's only -1 :P
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/QuotaManager.java
Line 433:
Line 434: /**
Line 435: * Roll back quota by VM id. the VM is fetched from DB and the
quota is rolled back
Line 436: * @param vmId - id for the vm
Line 437: */
use getQuotaDao
Line 438: public void rollbackQuotaByVmId(Guid vmId) {
Line 439: VM vm = DbFacade.getInstance().getVmDao().get(vmId);
Line 440: if (vm != null) {
Line 441: removeQuotaFromCache(vm.getStoragePoolId(),
vm.getQuotaId());
Line 759: public void updateUsage(List<Quota> quotaList) {
Line 760: List<Quota> needToCache = new ArrayList<Quota>();
Line 761:
Line 762: lock.readLock().lock();
Line 763: try {
please check for quotaList for null
Line 764: for (Quota quotaExternal : quotaList) {
Line 765: Quota quota = null;
Line 766: // look for the quota in the cache
Line 767: for (Map<Guid, Quota> quotaMap :
storagePoolQuotaMap.values()) {
Line 762: lock.readLock().lock();
Line 763: try {
Line 764: for (Quota quotaExternal : quotaList) {
Line 765: Quota quota = null;
Line 766: // look for the quota in the cache
this 'for' is redundant, quota has DC id.
Line 767: for (Map<Guid, Quota> quotaMap :
storagePoolQuotaMap.values()) {
Line 768: quota = quotaMap.get(quotaExternal.getId());
Line 769: if (quota != null) {
Line 770: break;
Line 787: lock.writeLock().lock();
Line 788: try {
Line 789: for (Quota quotaExternal : needToCache) {
Line 790: Quota quota = null;
Line 791: // look for the quota in the cache again (it may
have been added by now)
I think you must change this all chunk of code by simply calling
fetchQuotaFromCache? what do you say?
Line 792: for (Map<Guid, Quota> quotaMap :
storagePoolQuotaMap.values()) {
Line 793: quota = quotaMap.get(quotaExternal.getId());
Line 794: if (quota != null) {
Line 795: break;
Line 803:
storagePoolQuotaMap.put(quota.getStoragePoolId(), new HashMap<Guid, Quota>());
Line 804: }
Line 805:
storagePoolQuotaMap.get(quota.getStoragePoolId()).put(quota.getId(), quota);
Line 806: }
Line 807: }
nice :)
Line 808: copyUsageData(quota, quotaExternal);
Line 809: }
Line 810: } finally {
Line 811: lock.writeLock().unlock();
Line 863: Map<Guid, QuotaPerUserUsageEntity> quotaPerUserUsageEntityMap
= new HashMap<Guid, QuotaPerUserUsageEntity>();
Line 864: List<Quota> needToCache = new ArrayList<Quota>();
Line 865:
Line 866: lock.readLock().lock();
Line 867: try {
check for null (same) before the lock...
Line 868: for (Quota externalQuota : quotaIdsList) {
Line 869: Guid quotaId = externalQuota.getId();
Line 870: Quota quota = null;
Line 871: // look for the quota in the cache
Line 867: try {
Line 868: for (Quota externalQuota : quotaIdsList) {
Line 869: Guid quotaId = externalQuota.getId();
Line 870: Quota quota = null;
Line 871: // look for the quota in the cache
same, quota has storagepoolid, no need for iteration
Line 872: for (Map<Guid, Quota> quotaMap :
storagePoolQuotaMap.values()) {
Line 873:
Line 874: quota = quotaMap.get(quotaId);
Line 875: if (quota != null) {
Line 879:
Line 880: // if quota not in cache look for it in DB and add it
to cache
Line 881: if (quota == null) {
Line 882: needToCache.add(externalQuota);
Line 883: } else {
duplicate code, you have it in the end as well
Line 884: QuotaPerUserUsageEntity newEntity =
addQuotaEntry(quota);
Line 885: if (newEntity != null) {
Line 886: quotaPerUserUsageEntityMap.put(quotaId,
newEntity);
Line 887: }
Line 892: }
Line 893:
Line 894: if (!needToCache.isEmpty()) {
Line 895: lock.writeLock().lock();
Line 896: try {
same as before, use fetchQuotaFromCache
Line 897: for (Quota externalQuota : needToCache) {
Line 898: Guid quotaId = externalQuota.getId();
Line 899: Quota quota = null;
Line 900: // look for the quota in the cache again (it may
have been added by now)
Line 930:
Line 931: return new
ArrayList<QuotaPerUserUsageEntity>(quotaPerUserUsageEntityMap.values());
Line 932: }
Line 933:
Line 934: private void countPersonalUsage(List<VM> vms, Map<Guid,
QuotaPerUserUsageEntity> quotaPerUserUsageEntityMap) {
vms!=null?
Line 935: for (VM vm : vms) {
Line 936: // if vm is running and have a quota
Line 937: if (vm.getStatus() != VMStatus.Down
Line 938: && vm.getStatus() != VMStatus.Suspended
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/QuotaPerUserUsageEntity.java
Line 2: import org.ovirt.engine.core.compat.Guid;
Line 3:
Line 4: import java.io.Serializable;
Line 5:
Line 6: public class QuotaPerUserUsageEntity extends IVdcQueryable implements
Serializable {
please consider changing the name
Line 7:
Line 8: private Guid quotaId;
Line 9: private String quotaName;
Line 10: private double storageLimit;
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/GetQuotasConsumptionByAdElementIdQueryParameters.java
Line 9:
Line 10: /**
Line 11: * Auto generated serial id.
Line 12: */
Line 13: private static final long serialVersionUID = 4072642442090555682L;
maybe I'm wrong but don't you get the user from the session?
Line 14: private Guid adElementId;
Line 15: private Guid storagePoolId;
Line 16: private List<VM> vms;
Line 17:
--
To view, visit http://gerrit.ovirt.org/10120
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I67770aefec191832d0c5bb69fbaba82cd0e6febb
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: ofri masad <oma...@redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com>
Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com>
Gerrit-Reviewer: Michael Kublin <mkub...@redhat.com>
Gerrit-Reviewer: ofri masad <oma...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
