michael-o commented on code in PR #49:
URL: https://github.com/apache/velocity-engine/pull/49#discussion_r1735101505
##########
velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/CachingDatabaseObjectsFactory.java:
##########
@@ -0,0 +1,203 @@
+package org.apache.velocity.runtime. resource.loader;
+
+import org.apache.commons.pool2.BaseKeyedPooledObjectFactory;
+import org.apache.commons.pool2.KeyedObjectPool;
+import org.apache.commons.pool2.PooledObject;
+import org.apache.commons.pool2.impl.DefaultPooledObject;
+import org.apache.commons.pool2.impl.GenericKeyedObjectPool;
+import org.apache.commons.pool2.impl.GenericKeyedObjectPoolConfig;
+import org.apache.velocity.util.ExtProperties;
+import org.slf4j.Logger;
+
+import javax.sql.DataSource;
+import java.sql.Connection;
+import java.sql.PreparedStatement;
+import java.sql.SQLException;
+
+/**
+ * <p>Database objects factory which will keep a single connection to be able
to cache statements preparation, by means
+ * of appropriate pools.</p>
+ * <p>This class requires the following optional dependency (maven syntax):</p>
+ * <pre><code>
+ * <dependency>
+ * <groupId>org.apache.commons</groupId>
+ * <artifactId>commons-pool2</artifactId>
+ * <version>2.12.0</version>
+ * <scope>runtime</scope>
+ * </dependency>
+ * </code></pre>
+ * <p>To use this class, you must add the following property to the example
configuration described in
+ * @link{org.apache.velocity.runtime.resource.loader.DataSourceResourceLoader}
+ * </p>
+ * <pre><code>
+ * resource.loader.ds.database_objects_factory.class =
org.apache.velocity.runtime.resource.loader.DataSourceResourceLoader<br>
+ * </code></pre>
+ * <p>The default size of each pool of prepared statements (there is one pool
per statement) is 50. You can tune it
+ * with:</p>
+ * <pre><code>
+ * resource.loader.ds.database_objects_factory. =
org.apache.velocity.runtime.resource.loader.DataSourceResourceLoader<br>
+ * </code></pre>
+ * @see org.apache.velocity.runtime.resource.loader.DataSourceResourceLoader
+ */
+
+public class CachingDatabaseObjectsFactory implements DatabaseObjectsFactory {
+
+ private static final String STATEMENTS_POOL_MAX_SIZE =
"statements_pool_max_size";
+ private static final int STATEMENTS_POOL_MAX_SIZE_DEFAULT = 50;
+
+ private DataSource dataSource;
+ private Connection connection;
+ private int poolsMaxSize;
+ private KeyedObjectPool<String, PreparedStatement> statementsPool;
+ protected Logger log = null;
+
+ private class PreparedStatementFactory extends
BaseKeyedPooledObjectFactory<String, PreparedStatement>
+ {
+ @Override
+ public PreparedStatement create(String sql) throws Exception {
+ checkConnection();
+ return connection.prepareStatement(sql);
Review Comment:
Is this operation actually threadsafe? I mean one connection -- one thread,
but this create() method will be called by multiple threads in parallel.
##########
velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/CachingDatabaseObjectsFactory.java:
##########
@@ -0,0 +1,203 @@
+package org.apache.velocity.runtime. resource.loader;
+
+import org.apache.commons.pool2.BaseKeyedPooledObjectFactory;
+import org.apache.commons.pool2.KeyedObjectPool;
+import org.apache.commons.pool2.PooledObject;
+import org.apache.commons.pool2.impl.DefaultPooledObject;
+import org.apache.commons.pool2.impl.GenericKeyedObjectPool;
+import org.apache.commons.pool2.impl.GenericKeyedObjectPoolConfig;
+import org.apache.velocity.util.ExtProperties;
+import org.slf4j.Logger;
+
+import javax.sql.DataSource;
+import java.sql.Connection;
+import java.sql.PreparedStatement;
+import java.sql.SQLException;
+
+/**
+ * <p>Database objects factory which will keep a single connection to be able
to cache statements preparation, by means
+ * of appropriate pools.</p>
+ * <p>This class requires the following optional dependency (maven syntax):</p>
+ * <pre><code>
+ * <dependency>
+ * <groupId>org.apache.commons</groupId>
+ * <artifactId>commons-pool2</artifactId>
+ * <version>2.12.0</version>
+ * <scope>runtime</scope>
+ * </dependency>
+ * </code></pre>
+ * <p>To use this class, you must add the following property to the example
configuration described in
+ * @link{org.apache.velocity.runtime.resource.loader.DataSourceResourceLoader}
+ * </p>
+ * <pre><code>
+ * resource.loader.ds.database_objects_factory.class =
org.apache.velocity.runtime.resource.loader.DataSourceResourceLoader<br>
+ * </code></pre>
+ * <p>The default size of each pool of prepared statements (there is one pool
per statement) is 50. You can tune it
+ * with:</p>
+ * <pre><code>
+ * resource.loader.ds.database_objects_factory. =
org.apache.velocity.runtime.resource.loader.DataSourceResourceLoader<br>
+ * </code></pre>
+ * @see org.apache.velocity.runtime.resource.loader.DataSourceResourceLoader
+ */
+
+public class CachingDatabaseObjectsFactory implements DatabaseObjectsFactory {
+
+ private static final String STATEMENTS_POOL_MAX_SIZE =
"statements_pool_max_size";
+ private static final int STATEMENTS_POOL_MAX_SIZE_DEFAULT = 50;
+
+ private DataSource dataSource;
+ private Connection connection;
+ private int poolsMaxSize;
+ private KeyedObjectPool<String, PreparedStatement> statementsPool;
+ protected Logger log = null;
+
+ private class PreparedStatementFactory extends
BaseKeyedPooledObjectFactory<String, PreparedStatement>
+ {
+ @Override
+ public PreparedStatement create(String sql) throws Exception {
+ checkConnection();
+ return connection.prepareStatement(sql);
+ }
+
+ @Override
+ public PooledObject<PreparedStatement> wrap(PreparedStatement obj) {
+ return new DefaultPooledObject<>(obj);
+ }
+
+ @Override
+ public boolean validateObject(String key,
PooledObject<PreparedStatement> obj)
+ {
+ try
+ {
+ return !obj.getObject().isClosed() &&
obj.getObject().getConnection().isValid(0);
+ }
+ catch (SQLException sqle)
+ {
+ return false;
+ }
+ }
+
+ @Override
+ public void destroyObject(String key, PooledObject<PreparedStatement>
p) throws Exception {
+ p.getObject().close();
+ }
+ }
+
+ /**
+ * Initialize the factory with the DataSourceResourceLoader properties
+ * @param dataSource data source
+ */
+ @Override
+ public void init(DataSource dataSource, ExtProperties properties) throws
SQLException
+ {
+ this.dataSource = dataSource;
+ this.connection = dataSource.getConnection();
+ this.poolsMaxSize = properties.getInt(STATEMENTS_POOL_MAX_SIZE,
STATEMENTS_POOL_MAX_SIZE_DEFAULT);
+ createStatementsPool();
+ }
+
+ private void createStatementsPool()
+ {
+ GenericKeyedObjectPoolConfig<PreparedStatement> poolConfig = new
GenericKeyedObjectPoolConfig<>();
+ poolConfig.setMaxTotal(poolsMaxSize);
+ poolConfig.setTestOnBorrow(true);
+ statementsPool = new GenericKeyedObjectPool<>(new
PreparedStatementFactory(), poolConfig);
+ }
+
+ @Override
+ public void setLogger(Logger log)
+ {
+ this.log = log;
+ }
+
+ /**
+ * Prepare a statement
+ * @param sql Statement SQL
+ * @return prepared statement
+ */
+ @Override
+ public PreparedStatement prepareStatement(String sql) throws SQLException
+ {
+ try
+ {
+ return statementsPool.borrowObject(sql);
+ }
+ catch (SQLException sqle)
+ {
+ throw sqle;
+ }
+ catch (Exception e)
+ {
+ throw new SQLException("could not prepare statement", e);
+ }
+ }
+
+ protected synchronized void checkConnection() throws SQLException
+ {
+ if (!connection.isValid(0))
+ {
+ reset();
+ }
+ }
+
+ private void reset() throws SQLException
+ {
+ clear();
+ // refresh connection and statements pool
+ connection = dataSource.getConnection();
+ createStatementsPool();
+ }
+
+ /**
+ * Releases a prepared statement
+ * @param sql original sql query
+ * @param stmt statement
+ */
+ @Override
+ public void releaseStatement(String sql, PreparedStatement stmt) throws
SQLException
+ {
+ try
+ {
+ statementsPool.returnObject(sql, stmt);
+ }
+ catch (Exception e)
+ {
+ if (log != null)
+ {
+ log.debug("could not return statement to the pool", e);
+ }
+ }
+ }
+
+ /**
+ * Free resources
+ */
+ @Override
+ public void clear()
+ {
+ statementsPool.close();
Review Comment:
I mean, you kill you entire cache just because one object is dead.
##########
velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/CachingDatabaseObjectsFactory.java:
##########
@@ -0,0 +1,203 @@
+package org.apache.velocity.runtime. resource.loader;
+
+import org.apache.commons.pool2.BaseKeyedPooledObjectFactory;
+import org.apache.commons.pool2.KeyedObjectPool;
+import org.apache.commons.pool2.PooledObject;
+import org.apache.commons.pool2.impl.DefaultPooledObject;
+import org.apache.commons.pool2.impl.GenericKeyedObjectPool;
+import org.apache.commons.pool2.impl.GenericKeyedObjectPoolConfig;
+import org.apache.velocity.util.ExtProperties;
+import org.slf4j.Logger;
+
+import javax.sql.DataSource;
+import java.sql.Connection;
+import java.sql.PreparedStatement;
+import java.sql.SQLException;
+
+/**
+ * <p>Database objects factory which will keep a single connection to be able
to cache statements preparation, by means
+ * of appropriate pools.</p>
+ * <p>This class requires the following optional dependency (maven syntax):</p>
+ * <pre><code>
+ * <dependency>
+ * <groupId>org.apache.commons</groupId>
+ * <artifactId>commons-pool2</artifactId>
+ * <version>2.12.0</version>
+ * <scope>runtime</scope>
+ * </dependency>
+ * </code></pre>
+ * <p>To use this class, you must add the following property to the example
configuration described in
+ * @link{org.apache.velocity.runtime.resource.loader.DataSourceResourceLoader}
+ * </p>
+ * <pre><code>
+ * resource.loader.ds.database_objects_factory.class =
org.apache.velocity.runtime.resource.loader.DataSourceResourceLoader<br>
+ * </code></pre>
+ * <p>The default size of each pool of prepared statements (there is one pool
per statement) is 50. You can tune it
+ * with:</p>
+ * <pre><code>
+ * resource.loader.ds.database_objects_factory. =
org.apache.velocity.runtime.resource.loader.DataSourceResourceLoader<br>
+ * </code></pre>
+ * @see org.apache.velocity.runtime.resource.loader.DataSourceResourceLoader
+ */
+
+public class CachingDatabaseObjectsFactory implements DatabaseObjectsFactory {
+
+ private static final String STATEMENTS_POOL_MAX_SIZE =
"statements_pool_max_size";
+ private static final int STATEMENTS_POOL_MAX_SIZE_DEFAULT = 50;
+
+ private DataSource dataSource;
+ private Connection connection;
+ private int poolsMaxSize;
+ private KeyedObjectPool<String, PreparedStatement> statementsPool;
+ protected Logger log = null;
+
+ private class PreparedStatementFactory extends
BaseKeyedPooledObjectFactory<String, PreparedStatement>
+ {
+ @Override
+ public PreparedStatement create(String sql) throws Exception {
+ checkConnection();
+ return connection.prepareStatement(sql);
+ }
+
+ @Override
+ public PooledObject<PreparedStatement> wrap(PreparedStatement obj) {
+ return new DefaultPooledObject<>(obj);
+ }
+
+ @Override
+ public boolean validateObject(String key,
PooledObject<PreparedStatement> obj)
+ {
+ try
+ {
+ return !obj.getObject().isClosed() &&
obj.getObject().getConnection().isValid(0);
+ }
+ catch (SQLException sqle)
+ {
+ return false;
+ }
+ }
+
+ @Override
+ public void destroyObject(String key, PooledObject<PreparedStatement>
p) throws Exception {
+ p.getObject().close();
+ }
+ }
+
+ /**
+ * Initialize the factory with the DataSourceResourceLoader properties
+ * @param dataSource data source
+ */
+ @Override
+ public void init(DataSource dataSource, ExtProperties properties) throws
SQLException
+ {
+ this.dataSource = dataSource;
+ this.connection = dataSource.getConnection();
+ this.poolsMaxSize = properties.getInt(STATEMENTS_POOL_MAX_SIZE,
STATEMENTS_POOL_MAX_SIZE_DEFAULT);
+ createStatementsPool();
+ }
+
+ private void createStatementsPool()
+ {
+ GenericKeyedObjectPoolConfig<PreparedStatement> poolConfig = new
GenericKeyedObjectPoolConfig<>();
+ poolConfig.setMaxTotal(poolsMaxSize);
+ poolConfig.setTestOnBorrow(true);
+ statementsPool = new GenericKeyedObjectPool<>(new
PreparedStatementFactory(), poolConfig);
+ }
+
+ @Override
+ public void setLogger(Logger log)
+ {
+ this.log = log;
+ }
+
+ /**
+ * Prepare a statement
+ * @param sql Statement SQL
+ * @return prepared statement
+ */
+ @Override
+ public PreparedStatement prepareStatement(String sql) throws SQLException
+ {
+ try
+ {
+ return statementsPool.borrowObject(sql);
+ }
+ catch (SQLException sqle)
+ {
+ throw sqle;
+ }
+ catch (Exception e)
+ {
+ throw new SQLException("could not prepare statement", e);
+ }
+ }
+
+ protected synchronized void checkConnection() throws SQLException
+ {
+ if (!connection.isValid(0))
+ {
+ reset();
+ }
+ }
+
+ private void reset() throws SQLException
+ {
+ clear();
+ // refresh connection and statements pool
+ connection = dataSource.getConnection();
+ createStatementsPool();
+ }
Review Comment:
Why do you drain the entire pool if just one object is invalid? Wouldn't be
better to throw that object away?
##########
velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/DataSourceResourceLoader.java:
##########
@@ -119,6 +120,14 @@
* template_timestamp datetime NOT NULL
* );
* </code></pre>
+ * <p>Prior to Velocity 2.4, this class should not be considered
thread-safe.</p>
+ * <p>Since Velocity 2.4, the handling of JDBC connections and prepared
statements is delegated to the
+ * {@link org.apache.velocity.runtime.resource.loader.DatabaseObjectsFactory}
instance. The default class for this
+ * database objets factory is {@link
org.apache.velocity.runtime.resource.loader.DefaultDatabaseObjectsFactory},
+ * which obtain a new connection from the data source and prepare statements
at each query. You can configure this
Review Comment:
obtains, prepares with each...
##########
velocity-engine-core/src/main/java/org/apache/velocity/runtime/resource/loader/DataSourceResourceLoader.java:
##########
@@ -119,6 +120,14 @@
* template_timestamp datetime NOT NULL
* );
* </code></pre>
+ * <p>Prior to Velocity 2.4, this class should not be considered
thread-safe.</p>
+ * <p>Since Velocity 2.4, the handling of JDBC connections and prepared
statements is delegated to the
+ * {@link org.apache.velocity.runtime.resource.loader.DatabaseObjectsFactory}
instance. The default class for this
+ * database objets factory is {@link
org.apache.velocity.runtime.resource.loader.DefaultDatabaseObjectsFactory},
Review Comment:
objects
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
