New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HikariDataSource copy-constructor prevents inheriting configuration #1053

Closed
cowwoc opened this Issue Jan 1, 2018 · 7 comments

Comments

Projects
None yet
2 participants
@cowwoc

cowwoc commented Jan 1, 2018

Environment

HikariCP version: 2.7.4
JDK version     : 9.0.1+11
Database        : PostgreSQL
Driver version  : 42.1.4.jre7

Have you searched the CLOSED issues already? How about checking stackoverflow? Yes

#231 resulted in a public copy-constructor being added. Unfortunately, it doesn't work the way I expected it to.

I thought the entire point of the copy-constructor is to enable users to inherit configuration from an existing pool, then override some parameters, then launch the pool. Maybe this used to work in a past release, but it no longer does.

The copy-constructor is launching the new pool immediately. This means that any configuration changes requested after the copy constructor is invoked are being ignored silently.

Expected behavior:

  1. Users should be able to reconfigure the pool after invoking the copy-constructor, before pulling the first Connection.
  2. Configuration methods (e.g. setJdbcUrl()) should throw IllegalStateException if the pool is already started instead of ignoring such calls silently.
@cowwoc

This comment has been minimized.

Show comment
Hide comment
@cowwoc

cowwoc Jan 1, 2018

I was trying to track down at what point my code broke (used to work with configuration overriding but no longer does). Originally my code was invoking:

		HikariDataSource result = new HikariDataSource();
		result.setJdbcUrl(url1);
		// set some other parameters, then:
                doSomething1(result.getConnection())
                // then later on...
		result.setJdbcUrl(url2);
                doSomething2(result.getConnection())

and this used to work at some point. Somewhere down the line, the code changed and invoking setJdbcUrl() after pulling a connection no longer reconfigured the DataSource. So I tried using the copy-constructor to achieve the same goal and that failed too.

There are multiple issues here:

  1. Why does setJdbcUrl() get ignored? Is this by design?
  2. If so, can you please have it throw IllegalStateException to avoid silent failures?
  3. Should the copy-constructor spin up the pool right away? What's the point of this constructor if the configuration cannot be altered after construction?

cowwoc commented Jan 1, 2018

I was trying to track down at what point my code broke (used to work with configuration overriding but no longer does). Originally my code was invoking:

		HikariDataSource result = new HikariDataSource();
		result.setJdbcUrl(url1);
		// set some other parameters, then:
                doSomething1(result.getConnection())
                // then later on...
		result.setJdbcUrl(url2);
                doSomething2(result.getConnection())

and this used to work at some point. Somewhere down the line, the code changed and invoking setJdbcUrl() after pulling a connection no longer reconfigured the DataSource. So I tried using the copy-constructor to achieve the same goal and that failed too.

There are multiple issues here:

  1. Why does setJdbcUrl() get ignored? Is this by design?
  2. If so, can you please have it throw IllegalStateException to avoid silent failures?
  3. Should the copy-constructor spin up the pool right away? What's the point of this constructor if the configuration cannot be altered after construction?
@brettwooldridge

This comment has been minimized.

Show comment
Hide comment
@brettwooldridge

brettwooldridge Jan 4, 2018

Owner

@cowwoc The purpose of the "copy" is to ensure that changes to the configuration, through HikariConfig or subclasses (e.g. HikariDataSource), do not affect the operation of the pool once the pool is started. The only pool configuration that can be changed at runtime are those exposed via HikariConfigMXBean.

The purpose of the HikariDataSource(HikariConfig configuration) constructor is to support constructing a HikariDataSource with a HikariConfig, which is assumed to have been configured and the pool ready to start. The no-arg constructor originally did not exist, and HikariDataSource did not extend HikariConfig; it was only added to make Spring-based configuration more convenient.

I will take throwing IllegalStateException as an enhancement and one that makes perfect sense to me in service of the goal outlined above.

Owner

brettwooldridge commented Jan 4, 2018

@cowwoc The purpose of the "copy" is to ensure that changes to the configuration, through HikariConfig or subclasses (e.g. HikariDataSource), do not affect the operation of the pool once the pool is started. The only pool configuration that can be changed at runtime are those exposed via HikariConfigMXBean.

The purpose of the HikariDataSource(HikariConfig configuration) constructor is to support constructing a HikariDataSource with a HikariConfig, which is assumed to have been configured and the pool ready to start. The no-arg constructor originally did not exist, and HikariDataSource did not extend HikariConfig; it was only added to make Spring-based configuration more convenient.

I will take throwing IllegalStateException as an enhancement and one that makes perfect sense to me in service of the goal outlined above.

@cowwoc

This comment has been minimized.

Show comment
Hide comment
@cowwoc

cowwoc Jan 4, 2018

The purpose of the HikariDataSource(HikariConfig configuration) constructor is to support constructing a HikariDataSource with a HikariConfig, which is assumed to have been configured and the pool ready to start.

Makes sense. Can you please mention in the Javadoc that this constructor starts the pool right away? The no-op constructor Javadoc implies this indirectly, but users who read one Javadoc without the other are bound to miss it.

I will take throwing IllegalStateException as an enhancement and one that makes perfect sense to me in service of the goal outlined above.

Perfect, thank you.

cowwoc commented Jan 4, 2018

The purpose of the HikariDataSource(HikariConfig configuration) constructor is to support constructing a HikariDataSource with a HikariConfig, which is assumed to have been configured and the pool ready to start.

Makes sense. Can you please mention in the Javadoc that this constructor starts the pool right away? The no-op constructor Javadoc implies this indirectly, but users who read one Javadoc without the other are bound to miss it.

I will take throwing IllegalStateException as an enhancement and one that makes perfect sense to me in service of the goal outlined above.

Perfect, thank you.

@brettwooldridge

This comment has been minimized.

Show comment
Hide comment
@brettwooldridge

brettwooldridge Jan 5, 2018

Owner

@cowwoc Having looked at throwing IllegalStateException I believe that change would break some users. For example, because of the copy semantic, the HikariConfig is re-usable to start another pool, and users may be doing so. For example:

HikariConfig config = new HikariConfig();
config.setMaximumPoolSize(8);
config.setPoolName("Pool1");

HikariDataSource ds1 = new HikariDataSource(config);

config.setMaximumPoolSize(20);
config.setPoolName("Pool2");

HikariDataSource ds2 = new HikariDataSource(config);

"Sealing" the HikariConfig after first use is likely not an option, but I will certainly improve the JavaDoc around the constructor behavior.

Owner

brettwooldridge commented Jan 5, 2018

@cowwoc Having looked at throwing IllegalStateException I believe that change would break some users. For example, because of the copy semantic, the HikariConfig is re-usable to start another pool, and users may be doing so. For example:

HikariConfig config = new HikariConfig();
config.setMaximumPoolSize(8);
config.setPoolName("Pool1");

HikariDataSource ds1 = new HikariDataSource(config);

config.setMaximumPoolSize(20);
config.setPoolName("Pool2");

HikariDataSource ds2 = new HikariDataSource(config);

"Sealing" the HikariConfig after first use is likely not an option, but I will certainly improve the JavaDoc around the constructor behavior.

@cowwoc

This comment has been minimized.

Show comment
Hide comment
@cowwoc

cowwoc Jan 5, 2018

@brettwooldridge Maybe I missed something, but why is the above code problematic?

Specifically, although config is passed into ds1 and ds2's constructors, those constructors make a copy of the configuration and fire a pool against the copy. Hence, the original config is never sealed and there is no problem with modifying it.

My expectation for the above code is that ds1.setMaximumPoolSize() would throw an exception after the constructor, but config.setMaximumPoolSize() would not. Makes sense?

cowwoc commented Jan 5, 2018

@brettwooldridge Maybe I missed something, but why is the above code problematic?

Specifically, although config is passed into ds1 and ds2's constructors, those constructors make a copy of the configuration and fire a pool against the copy. Hence, the original config is never sealed and there is no problem with modifying it.

My expectation for the above code is that ds1.setMaximumPoolSize() would throw an exception after the constructor, but config.setMaximumPoolSize() would not. Makes sense?

@brettwooldridge

This comment has been minimized.

Show comment
Hide comment
@brettwooldridge

brettwooldridge Jan 5, 2018

Owner

@cowwoc The above code would be problematic, if, after HikariConfig is used to initialize ds1 it is "sealed" such that further calls to setters throw IllegalStateException. Having said that, I do (now) realize that it is possible to seal the HikariDataSource itself to disallow setter invocation. In summary, you're right. 😄

Owner

brettwooldridge commented Jan 5, 2018

@cowwoc The above code would be problematic, if, after HikariConfig is used to initialize ds1 it is "sealed" such that further calls to setters throw IllegalStateException. Having said that, I do (now) realize that it is possible to seal the HikariDataSource itself to disallow setter invocation. In summary, you're right. 😄

@cowwoc

This comment has been minimized.

Show comment
Hide comment
@cowwoc

cowwoc Jan 5, 2018

Excellent. Looks good except for a small typo: c48ccf6#r26665682

cowwoc commented Jan 5, 2018

Excellent. Looks good except for a small typo: c48ccf6#r26665682

brettwooldridge added a commit that referenced this issue Jan 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment