See https://github.com/brettwooldridge/HikariCP/issues/1096 for some background. We'll get a small performance boost in getConnection() and it'll also align us with the ideal usage of Hikari where the configuration is created and customised and then, once that's complete, it's used to create a HikariDataSource. The HikariDataSource then doesn't change (other than what's supported by Hikari's JMX integration).

Comment From: wilkinsona

This should be low risk and it'd future-proof our Hikari auto-configuration. I think we should consider doing this in 2.0.

Comment From: brettwooldridge

@wilkinsona Not only do users get a small boost in getConnection(), but across the board.

The IMetricsTracker APIs are called multiple times in a single acquire/release cycle. Currently, the set-after-initialize behavior can result in each of these call sites being inflated by the JVM into bimorphic call sites -- at roughly 2x the method dispatch cost. Initializing the pool fully, including the IMetricsTracker, will allow these call sites to remain monomorphic.

It might seem a small difference, ~100ns per connection cycle, but in high-performance applications with transaction rates in the thousands of TPS, it adds up. Actually, several times larger impact than the volatile access that is also avoided in getConnection().

Thanks for opening this tracking issue.

Comment From: snicoll

I agree that we should do our best to align with the philosophy of the libraries that we integrate and configuring HikariConfig to build the datasource is definitely going in the right direction. Quickly looking at both of them, the properties are almost the same but for loginTimeout (present in HikariDataSource but not HikariConfig). This is maybe an oversight (ping @brettwooldridge).

From a configuration perspective standpoint, we're good as dropping loginTimeout for 2.0 isn't the end of the World I guess and I am sure Brett can quickly fix that if that's an oversight (or gives us direction in case it isn't).

Having said that, there are several challenges:

  • The current programmatic model is based on DataSourceBuilder that generates a DataSource and is able to detect which implementation you want. This model intializes the DataSource with the basic bits with the clear intent that it will be further customized. Our examples of configuring a custom, or two datasources clearly are going in that direction. If we want to drop this mechanism, we need a replacement that would be Hikari-specific then as far as I can see
  • If we want to do this right, I think we need to create a HikariConfig bean (and ask Spring Boot to post-process it with the content of spring.datasource.hikari) only if no such bean already exists. Offering a way to back-off so that the user can define its own seems consistent with what we're doing in other areas. I don't think we should introduce that mechanism as we need proper user's feedback first

My vote is to defer this to 2.1.

Comment From: wilkinsona

I'd forgotten about the complexity introduced by DataSourceBuilder. I think that tips the balance and means this is too risky to consider for 2.0 at this very late stage.

Comment From: brettwooldridge

@wilkinsona @snicoll I agree, looking at the code, that the scope of the change seems far to large for the 2.0 time frame.

@snicoll Here's the low-down on loginTimeout. Basically, we strongly discourage the use of it. There are several reasons for this...

First, take the case of no pooling solution -- just straight JDBC.

1/ The loginTimeout is basically how users express how long they are willing to wait for a connection, as Connections are created immediately upon invocation of getConnection(). There is no other timeout.

2/ The setLoginTimeout() method is defined on the DataSource interface, but when using DriverManager-based configuration, the equivalent is DriverManager.setLoginTimeout(). This creates an issue for DriverManager-based applications, as the timeout is global to all drivers and databases. There is no way to have per-database timeouts in that case.

For this reason, most drivers define their own login/connection timeout property, independently.

In the context of a pool (like HikariCP), the connectionTimeout serves the purpose of loginTimeout -- i.e. how long the application is willing to wait for a connection.

In HikariCP, and some other pools, connection acquisition is asynchronous (on another thread), and so setting the loginTimeout would have no effect (or even detrimental effects).

HikariCP actually sets loginTimeout on the underlying DataSource to connectionTimeout upon initialization. We still permit the user to call loginTimeout on the HikariDataSource after pool initialization, which is a straight pass-thru to the underlying DataSource -- or to DriverManager.setLoginTimeout() in the case of a wrapped driver.

I would say this is highly discouraged, and we even considered just throwing an exception from setLoginTimeout(), but caution being the better part of valor, we left it in as an escape hatch (for who knows what...).

Long and short of it is, in the context of pools, its use should be avoided.

Comment From: kjhok1811

When configuring DataSource with Java Config, the previous procedure was as follows.

@Bean
@ConfigurationProperties(prefix ="spring.datasource.hikari")
public DataSource dataSource() {
   return DataSourceBuilder().create().type(HikariDataSource.class).build();
}

However, currently doing this does not reflect the jdbc connection information in the HikariDataSource.


@Bean
@ConfigurationProperties(prefix ="spring.datasource.hikari")
public HikariConfig hikariConfig() {
   return new HikariConfig();
}

@Bean
public DataSource dataSource() {
   return new HikariDataSource(hikariConfig());
}

It must be configured like this for it to work properly. If HikariDataSource is set to Java Config, is the creation process different from the existing method?

Comment From: snicoll

@kjhok1811 please review the guidelines for contributing, and ask questions on StackOverflow.

Comment From: ajmalab

Hey team, just curious, what happened to this issue? Was this decided against for any reason?

Comment From: scottfrederick

@ajmalab This issue is still open, and assigned to the "General Backlog" milestone. This means it is something that we still think has some value but it isn't a high priority so we haven't assigned it to a specific release and don't have firm plans to work on it.

Comment From: wilkinsona

The issue remains open so it hasn't been decided against. That said, it's not a priority for us at the moment hence it being in the general backlog milestone.