Maor Lipchuk has posted comments on this change.
Change subject: (WIP) Adding caching capabilities to database access
......................................................................
Patch Set 4: (10 inline comments)
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/cache/CacheableJdbcTemplate.java
Line 108: return result;
Line 109: }
Line 110:
Line 111: @Override
Line 112: public long queryForLong(String sql) throws DataAccessException {
Is it possible to use generics for all the queryForxxxx?
Line 113: Long result;
Line 114: result = (Long) cacheManager.getFromCache(sql, null);
Line 115:
Line 116: if (result == null) {
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/cache/CacheManager.java
Line 26: public static CacheManager getInstance() {
Line 27: return instance;
Line 28: }
Line 29:
Line 30: public Object getFromCache(String sql, Object[] args) {
I think it will be great if we can add an argument here which will indicate a
boolean of force refresh flag that if it is true we can return null, in case we
will want to update our cache at any time (Maybe in the future user can
configure that he doesn't want to use cache at all).
Line 31:
Line 32: if (log.isDebugEnabled())
Line 33: log.debug("Getting from cache for query " + sql);
Line 34:
Line 28: }
Line 29:
Line 30: public Object getFromCache(String sql, Object[] args) {
Line 31:
Line 32: if (log.isDebugEnabled())
You don't have to check if debug is enabled, that is already being checked when
calling log.debug
Line 33: log.debug("Getting from cache for query " + sql);
Line 34:
Line 35: CacheKey key = new CacheKey(sql, args);
Line 36:
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);
Consider to use log.debugFormat instead log.debug
Line 34:
Line 35: CacheKey key = new CacheKey(sql, args);
Line 36:
Line 37: CacheItemWrapper wrapper = cache.get(key);
Line 35: CacheKey key = new CacheKey(sql, args);
Line 36:
Line 37: CacheItemWrapper wrapper = cache.get(key);
Line 38: if (wrapper == null) {
Line 39: if (log.isDebugEnabled())
same here
Line 40: log.debug("Query " + sql + " not found in cache, "
Line 41: + Arrays.toString(args));
Line 42: return null;
Line 43: }
Line 42: return null;
Line 43: }
Line 44:
Line 45: if (wrapper.getData() == null) {
Line 46: if (log.isDebugEnabled())
same here
Line 47: log.debug("Query " + sql + " no data found in cache, "
Line 48: + Arrays.toString(args));
Line 49: return null;
Line 50: }
Line 58: type = dataList.get(0).getClass();
Line 59: }
Line 60:
Line 61: // Check cache time vs object TimeToLiveInCache definition
Line 62: TimeToLiveInCache ttlic = (TimeToLiveInCache) type
No need a cast here, casting is already being done in getAnnotation
implementation
Line 63: .getAnnotation(TimeToLiveInCache.class);
Line 64:
Line 65: // Calculate how long this object has been in cache
Line 66: long currentTime = System.currentTimeMillis();
Line 63: .getAnnotation(TimeToLiveInCache.class);
Line 64:
Line 65: // Calculate how long this object has been in cache
Line 66: long currentTime = System.currentTimeMillis();
Line 67: long diff = (currentTime - wrapper.getCacheTime()) / 1000;
Maybe it's better to declare a new constant for the value 1000
Also maybe it will be a bit more accurate to simply call
System.currentTimeMillis() at the calculation part of diff, such as this :
long diff = (System.currentTimeMillis() - wrapper.getCacheTime()) / 1000;
Line 68:
Line 69: // Default behavior when no annotation exists is to disable
cache
Line 70: if (ttlic == null || ttlic.value() < diff) {
Line 71: if (log.isDebugEnabled())
Line 109: for (CacheKey key : cacheKeys) {
Line 110: cache.remove(key);
Line 111: }
Line 112: typeToKeymapping.remove(updatedType);
Line 113: }
Perhaps consider to add an else here, that will log that the updatedType was
not found in the map.
Line 114: }
Line 115:
....................................................
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);
Please remove comment
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: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liran Zelkha <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Eli Mesika <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Laszlo Hornyak <[email protected]>
Gerrit-Reviewer: Liran Zelkha <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Mike Kolesnik <[email protected]>
Gerrit-Reviewer: Roy Golan <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
