Mike Kolesnik has posted comments on this change.
Change subject: (WIP) Adding caching capabilities to database access
......................................................................
Patch Set 4: (11 inline comments)
Please make sure the new code adheres to the rformatting by the formatter, and
to the Java code conventions such as that constant's names should be all CAPS.
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmStatic.java
Line 11: import org.ovirt.engine.core.common.validation.group.UpdateEntity;
Line 12: import org.ovirt.engine.core.compat.Guid;
Line 13: import org.ovirt.engine.core.compat.cache.TimeToLiveInCache;
Line 14:
Line 15: @TimeToLiveInCache(3000)
Did you mean to put 3000 seconds here?
Line 16: public class VmStatic extends VmBase {
Line 17: private static final long serialVersionUID = -2753306386502558044L;
Line 18:
Line 19: private Guid vmtGuid = new Guid();
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/cache/CacheableJdbcTemplate.java
Line 18: import org.springframework.jdbc.core.RowMapper;
Line 19: import org.springframework.jdbc.core.SqlParameter;
Line 20: import org.springframework.jdbc.support.rowset.SqlRowSet;
Line 21:
Line 22: public class CacheableJdbcTemplate extends JdbcTemplate {
Please remove all the auto comments:
// TODO Auto-generated method stub
Line 23:
Line 24: @SuppressWarnings("unused")
Line 25: private static final Log log = LogFactory
Line 26: .getLog(CacheableJdbcTemplate.class);
Line 243: @Override
Line 244: public <T> List<T> query(String sql, PreparedStatementSetter pss,
Line 245: RowMapper<T> rowMapper) throws DataAccessException {
Line 246:
Line 247: List<T> result;
Why declare result here?
Line 248: Object[] args = null;
Line 249:
Line 250: PreparedStatementMock psToCache = new PreparedStatementMock();
Line 251:
Line 252: try {
Line 253: pss.setValues(psToCache);
Line 254: args = psToCache.getArguments();
Line 255: } catch (SQLException e) {
Line 256: e.printStackTrace();
Seems to me like you should at least log here, and even rethrow the exception
or not use the cache in case of exception..
Line 257: }
Line 258:
Line 259: result = (List<T>) cacheManager.getFromCache(sql, args);
Line 260:
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/cache/CacheManager.java
Line 12:
Line 13: @SuppressWarnings("rawtypes")
Line 14: public class CacheManager {
Line 15: private static final Log log = LogFactory
Line 16: .getLog(CacheableJdbcTemplate.class);
Is the logging for a different class on purpose?
Line 17: private static CacheManager instance = new CacheManager();
Line 18:
Line 19: private Map<CacheKey, CacheItemWrapper> cache = new
WeakHashMap<CacheKey, CacheItemWrapper>();
Line 20: private Map<Class, List<CacheKey>> typeToKeymapping = new
WeakHashMap<Class, List<CacheKey>>();
Line 13: @SuppressWarnings("rawtypes")
Line 14: public class CacheManager {
Line 15: private static final Log log = LogFactory
Line 16: .getLog(CacheableJdbcTemplate.class);
Line 17: private static CacheManager instance = new CacheManager();
Why not declared as final?
Line 18:
Line 19: private Map<CacheKey, CacheItemWrapper> cache = new
WeakHashMap<CacheKey, CacheItemWrapper>();
Line 20: private Map<Class, List<CacheKey>> typeToKeymapping = new
WeakHashMap<Class, List<CacheKey>>();
Line 21:
Line 96: if (cacheKeys == null) {
Line 97: cacheKeys = new ArrayList<CacheKey>();
Line 98: }
Line 99: cacheKeys.add(key);
Line 100: typeToKeymapping.put(result.getClass(), cacheKeys);
There can be a race condition here, in which case the saved map won't be correct
Line 101:
Line 102: }
Line 103:
Line 104: // TODO: Should be synchronized for updatedType
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/cache/PreparedStatementMock.java
Line 24: import java.util.ArrayList;
Line 25: import java.util.Calendar;
Line 26: import java.util.List;
Line 27:
Line 28: public class PreparedStatementMock implements PreparedStatement {
Please remove all the auto comments:
// TODO Auto-generated method stub
Line 29: List<Object> params = new ArrayList<Object>();
Line 30:
Line 31: @Override
Line 32: public ResultSet executeQuery(String sql) throws SQLException {
Line 424:
Line 425: @Override
Line 426: public void setObject(int parameterIndex, Object x, int
targetSqlType)
Line 427: throws SQLException {
Line 428: params.add(x);
Are you sure only the "setObject" methods are used?
This is kinda tightly coupled to internal implementation details of Spring
JDBC..
Line 429: }
Line 430:
Line 431: @Override
Line 432: public void setObject(int parameterIndex, Object x) throws
SQLException {
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/PostgresDbEngineDialect.java
Line 25:
Line 26: /**
Line 27: * Postgres db engine dialect.
Line 28: */
Line 29: public class PostgresDbEngineDialect implements DbEngineDialect {
This file has mostly noise caused by running the auto-formatter. Please do this
in a preceding patch, so that we can see what the real changes are..
Line 30: @SuppressWarnings("unused")
Line 31: private static final Log log = LogFactory
Line 32: .getLog(PostgresDbEngineDialect.class);
Line 33: /**
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/SimpleJdbcCallsHandler.java
Line 100: return new CallCreator() {
Line 101: @Override
Line 102: public SimpleJdbcCall createCall() {
Line 103: // return new
SimpleJdbcCall(template).withProcedureName(procedureName);
Line 104: return new
CacheableJdbcCall(template).withProcedureName(procedureName);
Why would a modification call be cached?
Line 105: }
Line 106: };
Line 107: }
Line 108:
--
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 <liran.zel...@gmail.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Eli Mesika <emes...@redhat.com>
Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com>
Gerrit-Reviewer: Laszlo Hornyak <lhorn...@redhat.com>
Gerrit-Reviewer: Liran Zelkha <liran.zel...@gmail.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Michael Kublin <mkub...@redhat.com>
Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com>
Gerrit-Reviewer: Roy Golan <rgo...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
