Replace Apache commons-dbcp with HikariCP
As some benchmarks show [1], [2], [3], HikariCP is a lot faster than the
old commons-dcbp version inherited from the core dependencies, and with
minimal configuration: besides "sane" defaults, checking for invalid
connections is done automatically every time the pool is asked for a new
connection. Likewise, it tracks and closes abandoned connections, clears
connections before returning them to the client and resets auto-commit,
transaction isolation and read-only status for every connection.
Besides that, the library avoids locks as much as possible and the code
base is small. Projects like Liferay [4], Splunk, the Play framework and
even some Apache projects [5] use HikariCP.
[1] https://beansroasted.wordpress.com/tag/connection-pool-comparison/
[2] http://www.trustiv.co.uk/2014/06/battle-connection-pools
[3] https://github.com/brettwooldridge/HikariCP#jmh-benchmarks-checkered_flag
[4] https://community.liferay.com/blogs/-/blogs/tomcat-hikaricp
[5] https://github.com/brettwooldridge/HikariCP/blob/dev/documents/Wall-of-Fame.md
Change-Id: I79fe729e668fedf246f32b88a14a1c2c437cc8b0
diff --git a/BUILD b/BUILD
index cd34b32..04f201a 100644
--- a/BUILD
+++ b/BUILD
@@ -17,6 +17,7 @@
"Gerrit-HttpModule: com.ericsson.gerrit.plugins.eventslog.HttpModule",
],
resources = glob(["src/main/resources/**/*"]),
+ deps = ["@hikaricp//jar"],
)
junit_tests(
@@ -36,5 +37,6 @@
exports = PLUGIN_DEPS + PLUGIN_TEST_DEPS + [
":events-log__plugin",
"@mockito//jar",
+ "@hikaricp//jar",
],
)
diff --git a/external_plugin_deps.bzl b/external_plugin_deps.bzl
index 6650dc0..4ca722d 100644
--- a/external_plugin_deps.bzl
+++ b/external_plugin_deps.bzl
@@ -31,3 +31,9 @@
artifact = "org.objenesis:objenesis:2.6",
sha1 = "639033469776fd37c08358c6b92a4761feb2af4b",
)
+
+ maven_jar(
+ name = "hikaricp",
+ artifact = "com.zaxxer:HikariCP:3.2.0",
+ sha1 = "6c66db1c636ee90beb4c65fe34abd8ba9396bca6",
+ )
diff --git a/src/main/java/com/ericsson/gerrit/plugins/eventslog/EventsLogConfig.java b/src/main/java/com/ericsson/gerrit/plugins/eventslog/EventsLogConfig.java
index f4217bb..c1a9b4a 100644
--- a/src/main/java/com/ericsson/gerrit/plugins/eventslog/EventsLogConfig.java
+++ b/src/main/java/com/ericsson/gerrit/plugins/eventslog/EventsLogConfig.java
@@ -14,7 +14,6 @@
package com.ericsson.gerrit.plugins.eventslog;
-import com.google.common.base.Joiner;
import com.google.gerrit.extensions.annotations.PluginName;
import com.google.gerrit.server.config.PluginConfig;
import com.google.gerrit.server.config.PluginConfigFactory;
@@ -40,7 +39,6 @@
static final String CONFIG_PASSWORD = "storePassword";
static final String CONFIG_WAIT_TIME = "retryTimeout";
static final String CONFIG_CONN_TIME = "connectTimeout";
- static final String CONFIG_EVICT_IDLE_TIME = "evictIdleTime";
static final String CONFIG_MAX_CONNECTIONS = "maxConnections";
static final boolean DEFAULT_COPY_LOCAL = false;
@@ -49,7 +47,6 @@
static final int DEFAULT_RETURN_LIMIT = 5000;
static final int DEFAULT_WAIT_TIME = 1000;
static final int DEFAULT_CONN_TIME = 1000;
- static final int DEFAULT_EVICT_IDLE_TIME = 1000 * 60;
static final int DEFAULT_MAX_CONNECTIONS = 8;
private boolean copyLocal;
@@ -60,10 +57,9 @@
private int connectTime;
private String storeUrl;
private Path localStorePath;
- private String urlOptions;
+ private String[] urlOptions;
private String storeUsername;
private String storePassword;
- private int evictIdleTime;
private int maxConnections;
@Inject
@@ -80,10 +76,9 @@
Paths.get(
cfg.getString(
CONFIG_LOCAL_PATH, site.site_path.resolve("events-db").normalize().toString()));
- urlOptions = Joiner.on(";").join(cfg.getStringList(CONFIG_URL_OPTIONS));
+ urlOptions = cfg.getStringList(CONFIG_URL_OPTIONS);
storeUsername = cfg.getString(CONFIG_USERNAME);
storePassword = cfg.getString(CONFIG_PASSWORD);
- evictIdleTime = cfg.getInt(CONFIG_EVICT_IDLE_TIME, DEFAULT_EVICT_IDLE_TIME);
maxConnections = Math.max(cfg.getInt(CONFIG_MAX_CONNECTIONS, DEFAULT_MAX_CONNECTIONS), 1);
}
@@ -107,7 +102,7 @@
return storeUrl;
}
- public String getUrlOptions() {
+ public String[] getUrlOptions() {
return urlOptions;
}
@@ -131,10 +126,6 @@
return copyLocal;
}
- public int getEvictIdleTime() {
- return evictIdleTime;
- }
-
public int getMaxConnections() {
return maxConnections;
}
diff --git a/src/main/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLClient.java b/src/main/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLClient.java
index 2519eda..05f663f 100644
--- a/src/main/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLClient.java
+++ b/src/main/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLClient.java
@@ -33,6 +33,8 @@
import com.google.gerrit.server.events.SupplierSerializer;
import com.google.gson.Gson;
import com.google.gson.GsonBuilder;
+import com.zaxxer.hikari.HikariConfig;
+import com.zaxxer.hikari.HikariDataSource;
import java.sql.Connection;
import java.sql.ResultSet;
import java.sql.SQLException;
@@ -40,7 +42,6 @@
import java.sql.Timestamp;
import java.util.ArrayList;
import java.util.List;
-import org.apache.commons.dbcp.BasicDataSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -49,58 +50,13 @@
private final Gson gson;
private final boolean isPostgresql;
- private BasicDataSource ds;
+ private HikariDataSource ds;
- SQLClient(String storeUrl, String urlOptions) {
- ds = new BasicDataSource();
- ds.setUrl(storeUrl);
- ds.setConnectionProperties(urlOptions);
- ds.setMaxWait(MILLISECONDS.convert(30, SECONDS));
- ds.setTestOnBorrow(true);
- ds.setValidationQuery("SELECT 1");
- ds.setValidationQueryTimeout(5);
+ public SQLClient(HikariConfig config) {
+ ds = new HikariDataSource(config);
gson = new GsonBuilder().registerTypeAdapter(Supplier.class, new SupplierSerializer()).create();
- isPostgresql = storeUrl.contains("postgresql");
- }
-
- /**
- * Set the username to connect to the database.
- *
- * @param username the username as a string
- */
- void setUsername(String username) {
- ds.setUsername(username);
- }
-
- /**
- * Set the password to connect to the database.
- *
- * @param password the password as a string
- */
- void setPassword(String password) {
- ds.setPassword(password);
- }
-
- /**
- * Set the time before an idle connection is evicted as well as the time between eviction runs.
- *
- * @param evictIdleTime the time in milliseconds before eviction
- */
- void setEvictIdleTime(int evictIdleTime) {
- ds.setMinEvictableIdleTimeMillis(evictIdleTime);
- ds.setTimeBetweenEvictionRunsMillis(evictIdleTime / 2);
- }
-
- void setMaxConnections(int maxConnections) {
- ds.setMaxActive(maxConnections);
- ds.setMinIdle(maxConnections / 4);
- int maxIdle = maxConnections / 2;
- if (maxIdle == 0) {
- maxIdle = maxConnections;
- }
- ds.setMaxIdle(maxIdle);
- ds.setInitialSize(ds.getMinIdle());
+ isPostgresql = config.getJdbcUrl().contains("postgresql");
}
/**
@@ -127,11 +83,7 @@
}
void close() {
- try {
- ds.close();
- } catch (SQLException e) {
- log.warn("Cannot close datasource", e);
- }
+ ds.close();
}
/**
@@ -191,9 +143,9 @@
TABLE_NAME,
DATE_ENTRY,
new Timestamp(System.currentTimeMillis() - MILLISECONDS.convert(maxAge, DAYS))));
- log.info("Events older than {} days were removed from database", maxAge);
+ log.info("Events older than {} days were removed from database {}", maxAge, ds.getPoolName());
} catch (SQLException e) {
- log.warn("Cannot remove old event entries from database", e);
+ log.warn("Cannot remove old event entries from database {}", ds.getPoolName(), e);
}
}
diff --git a/src/main/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLModule.java b/src/main/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLModule.java
index 1088b15..df7f73b 100644
--- a/src/main/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLModule.java
+++ b/src/main/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLModule.java
@@ -20,11 +20,13 @@
import com.ericsson.gerrit.plugins.eventslog.EventStore;
import com.ericsson.gerrit.plugins.eventslog.EventsLogConfig;
import com.ericsson.gerrit.plugins.eventslog.QueryMaker;
+import com.google.gerrit.extensions.annotations.PluginName;
import com.google.gerrit.extensions.events.LifecycleListener;
import com.google.inject.AbstractModule;
import com.google.inject.Provides;
import com.google.inject.Singleton;
import com.google.inject.internal.UniqueAnnotations;
+import com.zaxxer.hikari.HikariConfig;
class SQLModule extends AbstractModule {
@@ -39,24 +41,34 @@
@Provides
@Singleton
@EventsDb
- SQLClient provideSqlClient(EventsLogConfig cfg) {
- SQLClient sqlClient = new SQLClient(cfg.getStoreUrl(), cfg.getUrlOptions());
- sqlClient.setUsername(cfg.getStoreUsername());
- sqlClient.setPassword(cfg.getStorePassword());
- sqlClient.setEvictIdleTime(cfg.getEvictIdleTime());
- sqlClient.setMaxConnections(cfg.getMaxConnections());
- return sqlClient;
+ SQLClient provideSqlClient(EventsLogConfig cfg, @PluginName String pluginName) {
+ HikariConfig dsConfig = new HikariConfig();
+ dsConfig.setJdbcUrl(cfg.getStoreUrl());
+ dsConfig.setUsername(cfg.getStoreUsername());
+ dsConfig.setPassword(cfg.getStorePassword());
+ dsConfig.setPoolName("[" + pluginName + "] EventsDb");
+ dsConfig.setMaximumPoolSize(cfg.getMaxConnections());
+ setDataSourceOptions(cfg, dsConfig);
+ return new SQLClient(dsConfig);
}
@Provides
@Singleton
@LocalEventsDb
- SQLClient provideLocalSqlClient(EventsLogConfig cfg) {
- SQLClient sqlClient =
- new SQLClient(
- H2_DB_PREFIX + cfg.getLocalStorePath().resolve(SQLTable.TABLE_NAME),
- cfg.getUrlOptions());
- sqlClient.setEvictIdleTime(cfg.getEvictIdleTime());
- return sqlClient;
+ SQLClient provideLocalSqlClient(EventsLogConfig cfg, @PluginName String pluginName) {
+ HikariConfig dsConfig = new HikariConfig();
+ dsConfig.setJdbcUrl(H2_DB_PREFIX + cfg.getLocalStorePath().resolve(SQLTable.TABLE_NAME));
+ dsConfig.setPoolName("[" + pluginName + "] LocalEventsDb");
+ setDataSourceOptions(cfg, dsConfig);
+ return new SQLClient(dsConfig);
+ }
+
+ private void setDataSourceOptions(EventsLogConfig cfg, HikariConfig dsConfig) {
+ for (String option : cfg.getUrlOptions()) {
+ int equalsPos = option.indexOf('=');
+ String key = option.substring(0, equalsPos);
+ String value = option.substring(equalsPos + 1);
+ dsConfig.addDataSourceProperty(key, value);
+ }
}
}
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md
index e019e8a..4421212 100644
--- a/src/main/resources/Documentation/config.md
+++ b/src/main/resources/Documentation/config.md
@@ -67,11 +67,6 @@
not be deleted and must be removed manually. When not specified, the default
value is set to false.
-plugin.@PLUGIN@.evictIdleTime
-: Interval of time in milliseconds after which an idle database connection is
- evicted from the connection pool. When not specified, the default value is
- set to 60000ms.
-
plugin.@PLUGIN@.maxConnections
: Maximum number of instances in the connection pool to the database. Includes
active and idle connections. By default 8.
diff --git a/src/test/java/com/ericsson/gerrit/plugins/eventslog/EventsLogConfigTest.java b/src/test/java/com/ericsson/gerrit/plugins/eventslog/EventsLogConfigTest.java
index 484d156..702328b 100644
--- a/src/test/java/com/ericsson/gerrit/plugins/eventslog/EventsLogConfigTest.java
+++ b/src/test/java/com/ericsson/gerrit/plugins/eventslog/EventsLogConfigTest.java
@@ -16,7 +16,6 @@
import static com.ericsson.gerrit.plugins.eventslog.EventsLogConfig.CONFIG_CONN_TIME;
import static com.ericsson.gerrit.plugins.eventslog.EventsLogConfig.CONFIG_COPY_LOCAL;
-import static com.ericsson.gerrit.plugins.eventslog.EventsLogConfig.CONFIG_EVICT_IDLE_TIME;
import static com.ericsson.gerrit.plugins.eventslog.EventsLogConfig.CONFIG_LOCAL_PATH;
import static com.ericsson.gerrit.plugins.eventslog.EventsLogConfig.CONFIG_MAX_AGE;
import static com.ericsson.gerrit.plugins.eventslog.EventsLogConfig.CONFIG_MAX_CONNECTIONS;
@@ -28,7 +27,6 @@
import static com.ericsson.gerrit.plugins.eventslog.EventsLogConfig.CONFIG_USERNAME;
import static com.ericsson.gerrit.plugins.eventslog.EventsLogConfig.CONFIG_WAIT_TIME;
import static com.ericsson.gerrit.plugins.eventslog.EventsLogConfig.DEFAULT_CONN_TIME;
-import static com.ericsson.gerrit.plugins.eventslog.EventsLogConfig.DEFAULT_EVICT_IDLE_TIME;
import static com.ericsson.gerrit.plugins.eventslog.EventsLogConfig.DEFAULT_MAX_AGE;
import static com.ericsson.gerrit.plugins.eventslog.EventsLogConfig.DEFAULT_MAX_CONNECTIONS;
import static com.ericsson.gerrit.plugins.eventslog.EventsLogConfig.DEFAULT_MAX_TRIES;
@@ -37,7 +35,6 @@
import static com.google.common.truth.Truth.assertThat;
import static org.mockito.Mockito.when;
-import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
import com.google.gerrit.server.config.PluginConfig;
import com.google.gerrit.server.config.PluginConfigFactory;
@@ -59,7 +56,6 @@
private static final String LOCAL_STORE_PATH = "~/gerrit/events-db/";
private static final String PLUGIN = "plugin";
private static final String PLUGIN_NAME = "eventsLog";
- private static final int CUSTOM_EVICT_IDLE_TIME = 10000;
private static final int CUSTOM_MAX_CONNECTIONS = 32;
private static final List<String> urlOptions = ImmutableList.of("DB_CLOSE_DELAY=10");
@@ -93,7 +89,6 @@
assertThat(eventsLogConfig.getUrlOptions()).isEmpty();
assertThat(eventsLogConfig.getStoreUsername()).isNull();
assertThat(eventsLogConfig.getStorePassword()).isNull();
- assertThat(eventsLogConfig.getEvictIdleTime()).isEqualTo(DEFAULT_EVICT_IDLE_TIME);
assertThat(eventsLogConfig.getMaxConnections()).isEqualTo(DEFAULT_MAX_CONNECTIONS);
}
@@ -110,10 +105,9 @@
assertThat(eventsLogConfig.getWaitTime()).isEqualTo(5000);
assertThat(eventsLogConfig.getLocalStorePath().toString() + "/").isEqualTo(LOCAL_STORE_PATH);
assertThat(eventsLogConfig.getStoreUrl()).isEqualTo("jdbc:h2:~/gerrit/db");
- assertThat(eventsLogConfig.getUrlOptions()).isEqualTo(Joiner.on(";").join(urlOptions));
+ assertThat(eventsLogConfig.getUrlOptions()).asList().isEqualTo(urlOptions);
assertThat(eventsLogConfig.getStoreUsername()).isEqualTo("testUsername");
assertThat(eventsLogConfig.getStorePassword()).isEqualTo("testPassword");
- assertThat(eventsLogConfig.getEvictIdleTime()).isEqualTo(CUSTOM_EVICT_IDLE_TIME);
assertThat(eventsLogConfig.getMaxConnections()).isEqualTo(CUSTOM_MAX_CONNECTIONS);
}
@@ -130,7 +124,6 @@
config.setStringList(PLUGIN, PLUGIN_NAME, CONFIG_URL_OPTIONS, urlOptions);
config.setString(PLUGIN, PLUGIN_NAME, CONFIG_USERNAME, "testUsername");
config.setString(PLUGIN, PLUGIN_NAME, CONFIG_PASSWORD, "testPassword");
- config.setInt(PLUGIN, PLUGIN_NAME, CONFIG_EVICT_IDLE_TIME, CUSTOM_EVICT_IDLE_TIME);
config.setInt(PLUGIN, PLUGIN_NAME, CONFIG_MAX_CONNECTIONS, CUSTOM_MAX_CONNECTIONS);
return config;
}
diff --git a/src/test/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLStoreTest.java b/src/test/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLStoreTest.java
index 3b0a19e..bbc4d05 100644
--- a/src/test/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLStoreTest.java
+++ b/src/test/java/com/ericsson/gerrit/plugins/eventslog/sql/SQLStoreTest.java
@@ -35,6 +35,7 @@
import com.google.gerrit.server.project.ProjectControl;
import com.google.gson.Gson;
import com.google.inject.Provider;
+import com.zaxxer.hikari.HikariConfig;
import java.io.IOException;
import java.net.ConnectException;
import java.sql.Connection;
@@ -77,6 +78,7 @@
private SQLClient eventsDb;
private SQLClient localEventsDb;
private SQLStore store;
+ private HikariConfig config;
private ScheduledThreadPoolExecutor poolMock;
private Statement stat;
@@ -85,6 +87,9 @@
@Before
public void setUp() throws SQLException {
+ config = new HikariConfig();
+ config.addDataSourceProperty("DB_CLOSE_DELAY", "-1");
+ config.addDataSourceProperty("DATABASE_TO_UPPER", "false");
Connection conn = DriverManager.getConnection(TEST_URL + ";" + TEST_OPTIONS);
stat = conn.createStatement();
poolMock = new PoolMock();
@@ -98,8 +103,10 @@
}
private void setUpClient() {
- eventsDb = new SQLClient(TEST_URL, TEST_OPTIONS);
- localEventsDb = new SQLClient(TEST_LOCAL_URL, TEST_OPTIONS);
+ config.setJdbcUrl(TEST_URL);
+ eventsDb = new SQLClient(config);
+ config.setJdbcUrl(TEST_LOCAL_URL);
+ localEventsDb = new SQLClient(config);
store =
new SQLStore(
pcFactoryMock,
@@ -313,8 +320,10 @@
when(pc.isVisible()).thenReturn(true);
doThrow(e).when(pcFactoryMock).controlFor((mockEvent3.getProjectNameKey()), userMock);
- eventsDb = new SQLClient(TEST_URL, TEST_OPTIONS);
- localEventsDb = new SQLClient(TEST_LOCAL_URL, TEST_OPTIONS);
+ config.setJdbcUrl(TEST_URL);
+ eventsDb = new SQLClient(config);
+ config.setJdbcUrl(TEST_LOCAL_URL);
+ localEventsDb = new SQLClient(config);
store =
new SQLStore(
pcFactoryMock,
@@ -404,7 +413,8 @@
*/
@Test
public void testConnectionTask() throws Exception {
- eventsDb = new SQLClient(TEST_URL, TEST_OPTIONS);
+ config.setJdbcUrl(TEST_URL);
+ eventsDb = new SQLClient(config);
localEventsDb = mock(SQLClient.class);
when(localEventsDb.dbExists()).thenReturn(true);
when(localEventsDb.getAll()).thenReturn(ImmutableList.of(mock(SQLEntry.class)));
@@ -435,7 +445,8 @@
private void checkConnectionAndRestore(boolean copy) throws Exception {
MockEvent mockEvent = new MockEvent();
eventsDb = mock(SQLClient.class);
- localEventsDb = new SQLClient(TEST_LOCAL_URL, TEST_OPTIONS);
+ config.setJdbcUrl(TEST_LOCAL_URL);
+ localEventsDb = new SQLClient(config);
localEventsDb.createDBIfNotCreated();
localEventsDb.storeEvent(mockEvent);
doThrow(new SQLException(new ConnectException()))
