Yair Zaslavsky has posted comments on this change.
Change subject: Adding caching capabilities to database access
......................................................................
Patch Set 1: (9 inline comments)
First comments...
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VdsDynamic.java
Line 13: import org.ovirt.engine.core.compat.StringHelper;
Line 14: import org.ovirt.engine.core.compat.Version;
Line 15: import org.ovirt.engine.core.compat.cache.TimeToLiveInCache;
Line 16:
Line 17: @TimeToLiveInCache(30)
How did you decide on the time to live values?
Line 18: public class VdsDynamic implements BusinessEntity<Guid> {
Line 19: private static final long serialVersionUID = -6010035855157006935L;
Line 20:
Line 21: private Guid id;
....................................................
File
backend/manager/modules/compat/src/main/java/org/ovirt/engine/core/compat/cache/TimeToLiveInCache.java
Line 6: import java.lang.annotation.Target;
Line 7:
Line 8: @Target(ElementType.TYPE)
Line 9: @Retention(RetentionPolicy.RUNTIME)
Line 10:
I would prefer this does not go into compat.
Should be at common.
Line 11: public @interface TimeToLiveInCache {
Line 12: int value();
Line 13:
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/cache/CacheItemWrapper.java
Line 1: package org.ovirt.engine.core.dal.cache;
Line 2:
Line 3: public class CacheItemWrapper {
What about having this as generic?
Line 4: private long cacheTime;
Line 5: private Object data;
Line 6:
Line 7: public CacheItemWrapper(long cacheTime, Object data) {
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/cache/CacheManager.java
Line 15: private static final Log log = LogFactory
Line 16: .getLog(CacheableJdbcTemplate.class);
Line 17: private static CacheManager instance = new CacheManager();
Line 18:
Line 19: private Map<CacheKey, CacheItemWrapper> cache = new
WeakHashMap<CacheKey, CacheItemWrapper>();
Why did you prefer WeakHashMap here?
Line 20: private Map<Class, List<CacheKey>> typeToKeymapping = new
WeakHashMap<Class, List<CacheKey>>();
Line 21:
Line 22: private CacheManager() {
Line 23:
Line 29:
Line 30: public Object getFromCache(String sql, Object[] args) {
Line 31:
Line 32: if (log.isDebugEnabled())
Line 33: log.debug("Getting from cache for query " + sql);
Matter of styling, in most of our code we will put this in a block.
Line 34:
Line 35: CacheKey key = new CacheKey(sql, args);
Line 36:
Line 37: CacheItemWrapper wrapper = cache.get(key);
Line 49: return null;
Line 50: }
Line 51:
Line 52: Class<? extends Object> type = wrapper.getData().getClass();
Line 53:
I would add a comment about the "calculate type of list" thing you do.
In addition, why not support Collection, and maps? is it because all our DAOs
return either single entity or a list of entities?
Line 54: if (List.class.isAssignableFrom(type)) {
Line 55:
Line 56: List dataList = (List) wrapper.getData();
Line 57: if (dataList.size() > 0)
Line 101:
Line 102: }
Line 103:
Line 104: // TODO: Should be synchronized for updatedType
Line 105: public void cleanCacheForType(Class updatedType) {
Probably should be Class<?> and not Class, right, and then we can remove the
@SupressWarnings? It's up to you.
Line 106: List<CacheKey> cacheKeys = typeToKeymapping.get(updatedType);
Line 107:
Line 108: if (cacheKeys != null) {
Line 109: for (CacheKey key : cacheKeys) {
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/PostgresDbEngineDialect.java
Line 26: /**
Line 27: * Postgres db engine dialect.
Line 28: */
Line 29: public class PostgresDbEngineDialect implements DbEngineDialect {
Line 30: @SuppressWarnings("unused")
Are you using the code formatter file with Eclipse? (Or maybe at the "original"
version formatting wasn't used? it's hard to read the changes this way. It
looks like the only changes are the @SupressWarnings and the fact that
PostgresJdbcTemplate extends now from CacheableJdbcTemplate. Am I missing
something ?
Line 31: private static final Log log = LogFactory
Line 32: .getLog(PostgresDbEngineDialect.class);
Line 33: /**
Line 34: * Prefix used in our PostgreSQL function parameters.
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/SimpleJdbcCallsHandler.java
Line 99: private CallCreator createCallForModification(final String
procedureName) {
Line 100: return new CallCreator() {
Line 101: @Override
Line 102: public SimpleJdbcCall createCall() {
Line 103: // return new
SimpleJdbcCall(template).withProcedureName(procedureName);
You can remove this comment, if needed we will use git log etc..
Line 104: return new
CacheableJdbcCall(template).withProcedureName(procedureName);
Line 105: }
Line 106: };
Line 107: }
--
To view, visit http://gerrit.ovirt.org/14188
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I04d7edea6a52626c68f88362416abdb025e4b026
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liran Zelkha <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Roy Golan <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
