Gilad Chaplik has posted comments on this change.
Change subject: core: Fast init cache
......................................................................
Patch Set 6: I would prefer that you didn't submit this
(12 inline comments)
some more comments to follow;
....................................................
File backend/manager/dbscripts/quota_sp.sql
Line 70: Create or replace FUNCTION GetQuotaCount()
Line 71: RETURNS SETOF INTEGER
Line 72: AS $procedure$
Line 73: BEGIN
Line 74: RETURN QUERY SELECT cast(count(*) as INTEGER) as num_quota
why casting here?
Line 75: FROM quota;
Line 76: END; $procedure$
Line 77: LANGUAGE plpgsql;
Line 78:
Line 73: BEGIN
Line 74: RETURN QUERY SELECT cast(count(*) as INTEGER) as num_quota
Line 75: FROM quota;
Line 76: END; $procedure$
Line 77: LANGUAGE plpgsql;
no need to count, this SP can be removed
you can get the count from the getAll....
and update the cached count using add/remove Quota commands.
Line 78:
Line 79: Create or replace FUNCTION GetQuotaStorageByQuotaGuid(v_id UUID)
Line 80: RETURNS SETOF quota_storage_view
Line 81: AS $procedure$
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/quota/QuotaManager.java
Line 30:
Line 31: private static final QuotaManagerAuditLogger
quotaManagerAuditLogger = new QuotaManagerAuditLogger();
Line 32: private List<QuotaConsumptionParameter> corruptedParameters = new
ArrayList<QuotaConsumptionParameter>();
Line 33: private static final double MINIMUM_CACHE_FACTOR =
Line 34: (double) Config.<Integer>
GetValue(ConfigValues.MinimumPercentageToUpdateQuotaCache) / 100;
missing comment/better constant name
Line 35:
Line 36: public static QuotaManager getInstance() {
Line 37: return INSTANCE;
Line 38: }
Line 986:
Line 987: /**
Line 988: * InitializeCache is called by SchedulerUtilQuartzImpl.
Line 989: */
Line 990: @OnTimerMethodAnnotation("initializeCache")
+1
updateCache
Line 991: public void initializeCache() {
Line 992: if (!isNeedToInitializeCache()) {
Line 993: return;
Line 994: }
Line 987: /**
Line 988: * InitializeCache is called by SchedulerUtilQuartzImpl.
Line 989: */
Line 990: @OnTimerMethodAnnotation("initializeCache")
Line 991: public void initializeCache() {
What is the policy for stacking calls to this method?
Line 992: if (!isNeedToInitializeCache()) {
Line 993: return;
Line 994: }
Line 995:
Line 992: if (!isNeedToInitializeCache()) {
Line 993: return;
Line 994: }
Line 995:
Line 996: log.info("Updating Quota Cache...");
+1
Line 997: long timeStart = System.currentTimeMillis();
Line 998: List<Quota> allQuotaIncludingConsumption =
getQuotaDAO().getAllQuotaIncludingConsumption();
Line 999:
Line 1000: if (allQuotaIncludingConsumption.isEmpty()) {
Line 994: }
Line 995:
Line 996: log.info("Updating Quota Cache...");
Line 997: long timeStart = System.currentTimeMillis();
Line 998: List<Quota> allQuotaIncludingConsumption =
getQuotaDAO().getAllQuotaIncludingConsumption();
small Q; why not to set the quota into DC map in the DAO layer? you already
iterates it there, and it's not inside a critical section as in the next lines.
Line 999:
Line 1000: if (allQuotaIncludingConsumption.isEmpty()) {
Line 1001: return;
Line 1002: }
Line 1016: log.infoFormat("Quota Cache updated. ({0} msec)",
timeEnd-timeStart);
Line 1017: }
Line 1018:
Line 1019: public boolean isNeedToInitializeCache() {
Line 1020: int quotaCount = getQuotaDAO().getQuotaCount();
no need to go to DB, see my comment in createSP file
Line 1021: int cacheCount = 0;
Line 1022:
Line 1023: lock.readLock().lock();
Line 1024: try {
Line 1023: lock.readLock().lock();
Line 1024: try {
Line 1025: for(Map<Guid, Quota> quotaMap :
storagePoolQuotaMap.values()) {
Line 1026: cacheCount += quotaMap.size();
Line 1027: }
I really can't understand why you need to count here? you hold and manage it
all the time. and inside a lock it's even ...
Line 1028: } finally {
Line 1029: lock.readLock().unlock();
Line 1030: }
Line 1031:
Line 1028: } finally {
Line 1029: lock.readLock().unlock();
Line 1030: }
Line 1031:
Line 1032: return cacheCount < quotaCount * MINIMUM_CACHE_FACTOR;
not sure I agree or understand the entire factor issue, users will set it to
what? please justify this config val from users perspective
Line 1033: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java
Line 1306:
Line 1307: @TypeConverterAttribute(Integer.class)
Line 1308: @DefaultValueAttribute("60")
Line 1309: MinimumPercentageToUpdateQuotaCache(408),
Line 1310:
:-)
Line 1311: Invalid(65535);
Line 1312:
Line 1313: private int intValue;
Line 1314: private static Map<Integer, ConfigValues> mappings;
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/QuotaDAODbFacadeImpl.java
Line 212: allQuotaMap.put(quota.getId(), quota);
Line 213: }
Line 214:
Line 215: List<QuotaStorage> quotaStorageList =
getAllQuotaStorageIncludingConsumption();
Line 216: List<QuotaVdsGroup> quotaVdsGroupList =
getALLQuotaVdsGroupIncludingConsumption();
ALL/all; consistency (I prefer All :))
Line 217:
Line 218: if (quotaStorageList != null) {
Line 219: for (QuotaStorage quotaStorage : quotaStorageList) {
Line 220: Quota quota =
allQuotaMap.get(quotaStorage.getQuotaId());
--
To view, visit http://gerrit.ovirt.org/10159
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id3db08957e413d2f1e0480b764334dd7268c8221
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: ofri masad <[email protected]>
Gerrit-Reviewer: Doron Fediuck <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Michael Kublin <[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
