Add db connection pool support #791
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideIntroduces optional JDBC connection pooling with HikariCP based on a new maxPoolSize setting, updating configuration and connection manager logic, adding the required dependency, and covering behavior with new tests. Sequence diagram for connection creation with and without poolingsequenceDiagram
participant "Client"
participant "JdbcConnectionManager"
participant "HikariCP Pool"
participant "Database"
alt Pooling enabled (maxPoolSize > 0)
"Client"->>"JdbcConnectionManager": getJdbi()
"JdbcConnectionManager"->>"HikariCP Pool": request connection
"HikariCP Pool"->>"Database": get connection
"HikariCP Pool"-->>"JdbcConnectionManager": pooled connection
"JdbcConnectionManager"-->>"Client": Jdbi instance
else Pooling disabled (maxPoolSize is null)
"Client"->>"JdbcConnectionManager": getJdbi()
"JdbcConnectionManager"->>"Database": create connection
"Database"-->>"JdbcConnectionManager": connection
"JdbcConnectionManager"-->>"Client": Jdbi instance
end
ER diagram for DataStoreConfiguration with new maxPoolSize fielderDiagram
DATASTORECONFIGURATION {
String jdbcUrl
String user
String password
String driver
Integer queryHistoryHoursRetention
boolean runMigrationsEnabled
Integer maxPoolSize
}
Class diagram for updated DataStoreConfiguration and JdbcConnectionManagerclassDiagram
class DataStoreConfiguration {
-String jdbcUrl
-String user
-String password
-String driver
-Integer queryHistoryHoursRetention
-boolean runMigrationsEnabled
-Integer maxPoolSize
+DataStoreConfiguration(jdbcUrl, user, password, driver, queryHistoryHoursRetention, runMigrationsEnabled, maxPoolSize)
+DataStoreConfiguration(jdbcUrl, user, password, driver, queryHistoryHoursRetention, runMigrationsEnabled)
+getMaxPoolSize()
+setMaxPoolSize(maxPoolSize)
}
class JdbcConnectionManager {
+Jdbi getJdbi(routingGroupDatabase)
}
DataStoreConfiguration <.. JdbcConnectionManager : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Rather than creating a new HikariDataSource on every getJdbi() call, initialize and cache a single pooled DataSource per routing group so you don’t spawn multiple pools and can manage its lifecycle more cleanly.
- Add a shutdown or close hook for the HikariDataSource instances to ensure the connection pool is properly terminated and avoids resource leaks when the application stops.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Rather than creating a new HikariDataSource on every getJdbi() call, initialize and cache a single pooled DataSource per routing group so you don’t spawn multiple pools and can manage its lifecycle more cleanly.
- Add a shutdown or close hook for the HikariDataSource instances to ensure the connection pool is properly terminated and avoids resource leaks when the application stops.
## Individual Comments
### Comment 1
<location> `gateway-ha/src/main/java/io/trino/gateway/ha/persistence/JdbcConnectionManager.java:66-72` </location>
<code_context>
+ if (configuration.getMaxPoolSize() != null && configuration.getMaxPoolSize() > 0) {
+ HikariConfig hikariConfig = new HikariConfig();
+ hikariConfig.setJdbcUrl(buildJdbcUrl(routingGroupDatabase));
+ hikariConfig.setUsername(configuration.getUser());
+ hikariConfig.setPassword(configuration.getPassword());
+ if (configuration.getDriver() != null) {
+ hikariConfig.setDriverClassName(configuration.getDriver());
+ }
+ hikariConfig.setMaximumPoolSize(configuration.getMaxPoolSize());
+ return Jdbi.create(new HikariDataSource(hikariConfig))
+ .installPlugin(new SqlObjectPlugin())
</code_context>
<issue_to_address>
**suggestion (performance):** HikariConfig is instantiated for every call; consider reusing or pooling.
Repeatedly creating HikariConfig and HikariDataSource instances can waste resources and fragment connection pools. Reuse the HikariDataSource or manage its lifecycle to improve efficiency.
Suggested implementation:
```java
if (configuration.getMaxPoolSize() != null && configuration.getMaxPoolSize() > 0) {
return Jdbi.create(getOrCreateHikariDataSource(routingGroupDatabase, configuration))
.installPlugin(new SqlObjectPlugin())
.registerRowMapper(new RecordAndAnnotatedConstructorMapper());
}
```
```java
import com.zaxxer.hikari.HikariConfig;
import com.zaxxer.hikari.HikariDataSource;
import io.airlift.log.Logger;
import io.trino.gateway.ha.config.DataStoreConfiguration;
import io.trino.gateway.ha.persistence.dao.QueryHistoryDao;
import java.util.concurrent.atomic.AtomicReference;
private final AtomicReference<HikariDataSource> hikariDataSourceRef = new AtomicReference<>();
private HikariDataSource getOrCreateHikariDataSource(String routingGroupDatabase, DataStoreConfiguration configuration) {
HikariDataSource existing = hikariDataSourceRef.get();
if (existing != null && !existing.isClosed()) {
return existing;
}
synchronized (hikariDataSourceRef) {
existing = hikariDataSourceRef.get();
if (existing != null && !existing.isClosed()) {
return existing;
}
HikariConfig hikariConfig = new HikariConfig();
hikariConfig.setJdbcUrl(buildJdbcUrl(routingGroupDatabase));
hikariConfig.setUsername(configuration.getUser());
hikariConfig.setPassword(configuration.getPassword());
if (configuration.getDriver() != null) {
hikariConfig.setDriverClassName(configuration.getDriver());
}
hikariConfig.setMaximumPoolSize(configuration.getMaxPoolSize());
HikariDataSource newDataSource = new HikariDataSource(hikariConfig);
hikariDataSourceRef.set(newDataSource);
return newDataSource;
}
}
// Optionally, add a close method to clean up the datasource
public void close() {
HikariDataSource ds = hikariDataSourceRef.getAndSet(null);
if (ds != null && !ds.isClosed()) {
ds.close();
}
}
return jdbi;
}
```
- You may need to ensure thread safety and proper lifecycle management for the `HikariDataSource` depending on how `JdbcConnectionManager` is used.
- If the manager is used for multiple databases/configurations, you may need to use a map keyed by database/configuration instead of a single reference.
- Make sure to call the `close()` method when the manager is shut down to release resources.
- If this is a singleton or managed bean, consider integrating with your application's lifecycle management.
</issue_to_address>
### Comment 2
<location> `gateway-ha/src/test/java/io/trino/gateway/ha/persistence/TestJdbcConnectionManagerPool.java:15` </location>
<code_context>
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+final class TestJdbcConnectionManagerPool {
+ @Test
+ void blocksWhenExceedingMaxPoolSize() throws Exception {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for invalid maxPoolSize values (e.g., zero, negative).
Tests only cover null and positive maxPoolSize. Please add cases for zero and negative values to ensure correct fallback or error handling.
Suggested implementation:
```java
final class TestJdbcConnectionManagerPool {
@Test
void blocksWhenExceedingMaxPoolSize() throws Exception {
String dbPath = Path.of(System.getProperty("java.io.tmpdir"), "h2db-pool-" + System.currentTimeMillis()).toString();
String jdbcUrl = "jdbc:h2:" + dbPath;
DataStoreConfiguration cfg = new DataStoreConfiguration(
jdbcUrl, "sa", "sa", "org.h2.Driver",
4, true,
2
);
JdbcConnectionManager cm = new JdbcConnectionManager(Jdbi.create(jdbcUrl, "sa", "sa"), cfg);
}
@Test
void handlesZeroMaxPoolSize() {
String dbPath = Path.of(System.getProperty("java.io.tmpdir"), "h2db-pool-zero-" + System.currentTimeMillis()).toString();
String jdbcUrl = "jdbc:h2:" + dbPath;
DataStoreConfiguration cfg = new DataStoreConfiguration(
jdbcUrl, "sa", "sa", "org.h2.Driver",
4, true,
0
);
try {
JdbcConnectionManager cm = new JdbcConnectionManager(Jdbi.create(jdbcUrl, "sa", "sa"), cfg);
// Depending on implementation, either fallback to default or throw
// If fallback, check pool size is not zero
assertThat(cm.getMaxPoolSize()).isGreaterThan(0);
} catch (Exception e) {
// If exception is expected, assert it is thrown
assertThat(e).isInstanceOfAny(IllegalArgumentException.class, RuntimeException.class);
}
}
@Test
void handlesNegativeMaxPoolSize() {
String dbPath = Path.of(System.getProperty("java.io.tmpdir"), "h2db-pool-negative-" + System.currentTimeMillis()).toString();
String jdbcUrl = "jdbc:h2:" + dbPath;
DataStoreConfiguration cfg = new DataStoreConfiguration(
jdbcUrl, "sa", "sa", "org.h2.Driver",
4, true,
-5
);
try {
JdbcConnectionManager cm = new JdbcConnectionManager(Jdbi.create(jdbcUrl, "sa", "sa"), cfg);
// Depending on implementation, either fallback to default or throw
// If fallback, check pool size is not negative
assertThat(cm.getMaxPoolSize()).isGreaterThan(0);
} catch (Exception e) {
// If exception is expected, assert it is thrown
assertThat(e).isInstanceOfAny(IllegalArgumentException.class, RuntimeException.class);
}
}
```
- If `JdbcConnectionManager` does not have a `getMaxPoolSize()` method, you should add it or otherwise access the pool size for assertion.
- Adjust the exception types in the assertions to match the actual exceptions thrown by your implementation.
- If your implementation logs or handles these cases differently, update the assertions accordingly.
</issue_to_address>
### Comment 3
<location> `gateway-ha/src/test/java/io/trino/gateway/ha/persistence/TestJdbcConnectionManagerPool.java:84` </location>
<code_context>
+
+
+ @Test
+ void doesNotBlockWhenMaxPoolSizeIsNull() throws Exception {
+ String dbPath = Path.of(System.getProperty("java.io.tmpdir"), "h2db-nopool-" + System.currentTimeMillis()).toString();
+ String jdbcUrl = "jdbc:h2:" + dbPath;
</code_context>
<issue_to_address>
**suggestion (testing):** Consider testing with a very high maxPoolSize to ensure no artificial bottleneck.
Add a test with maxPoolSize set to a high value (e.g., 1000) to verify that connections remain non-blocking under high concurrency.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
gateway-ha/src/main/java/io/trino/gateway/ha/persistence/JdbcConnectionManager.java
Outdated
Show resolved
Hide resolved
arturmkr
force-pushed
the
feature/add-db-connection-pool-support
branch
from
e52be50 to
5ac762c
Compare
arturmkr
force-pushed
the
feature/add-db-connection-pool-support
branch
from
5ac762c to
e7565fa
Compare
rdsarvar
reviewed
Nov 4, 2025
| private String driver; | ||
| private Integer queryHistoryHoursRetention = 4; | ||
| private boolean runMigrationsEnabled = true; | ||
| private Integer maxPoolSize; |
Contributor
There was a problem hiding this comment.
nit: IMO if we're adding HikariCP we should be giving the users complete flexibility to all of it's settings and not just max pool size:
Example of what I mean from previously unpushed code I had laying around:
+ // HikariCP connection pool settings
+ private int maximumPoolSize = 10;
+ private int minimumIdle = 5;
+ private long connectionTimeout = 30000; // 30 seconds
+ private long idleTimeout = 600000; // 10 minutes
+ private long maxLifetime = 1800000; // 30 minutes
+ private long leakDetectionThreshold = 60000; // 1 minute
+ private boolean autoCommit = true;
+ private String connectionTestQuery;
For the above to work with your logic max pool size can default to 0
Author
There was a problem hiding this comment.
Thanks for the suggestion! I think exposing all HikariCP parameters would add unnecessary complexity for this use case as Hikari’s default settings are already well-optimized and sufficient.
For now I kept it minimal with maxPoolSize, since it’s the only setting that typically needs adjustment.
| }); | ||
| } | ||
|
|
||
| public void close() |
Contributor
There was a problem hiding this comment.
question: is this only used by tests? I don't see where it's being called otherwise
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduces optional JDBC connection pooling using HikariCP in
JdbcConnectionManager, solves issue #377Details
maxPoolSizetoDataStoreConfiguration.maxPoolSizeis configured (non-null and > 0):JdbcConnectionManageruses a Hikari connection pool for routing-group database connections.maxPoolSizeis not set (null):Tests
Added
TestJdbcConnectionManagerPool:Why
This allows controlling connection usage and improving DB efficiency under load, while keeping the old behavior as the default (no pooling).
Summary by Sourcery
Introduce optional JDBC connection pooling using HikariCP controlled by a new maxPoolSize setting, while preserving default direct connections, and add tests to verify pooling behavior
New Features:
Enhancements:
Build:
Tests: