Skip to content

Added support for data source proxying. #8753

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

Conversation

gavlyukovskiy
Copy link
Contributor

@gavlyukovskiy gavlyukovskiy commented Mar 25, 2017

Autoconfigured to indicate p6spy, datasource-proxy, flexy-pool by default and wrap autoconfigured data source.
Supported to define custom data source proxy or change order of proxying if multimple libraries found using spring.datasource.proxyType, disables proxying if empty.
Added ProxyDataSourcePoolMetadataProvider to get metrics from the real data source.

Fixes #7863.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 25, 2017
@philwebb
Copy link
Member

Thanks for the PR. Unfortunately I think that there might be a few problems with the approach.

The first is that the existing datasource beans that get exposed are bound to @ConfigurationProperties. If we return proxy variant of those beans then the properties won't get bound correctly.

The second problem is that the DataSourceBuilder is now automatically creating proxy DataSource instances. I think that this something that the user should opt into at the builder level, rather than it being automatic.

I'm not sure on the best way to fix these issue. Perhaps we can use a BeanPostProcessor to replace the DataSource bean as late as possible. That would also remove the need for all the additional pool specific DataSource auto-configuration.

@philwebb philwebb added the status: on-hold We can't start working on this issue yet label Mar 27, 2017
@vpavic
Copy link
Contributor

vpavic commented Mar 28, 2017

Perhaps we can use a BeanPostProcessor to replace the DataSource bean as late as possible. That would also remove the need for all the additional pool specific DataSource auto-configuration.

I've started sketching something along those lines a few weeks ago but didn't have time to round it off into a PR. My approach was to introduce DataSourceDecorator contract and provide some implementations out of the box (for proxy-datasource, p6spy, etc) while also allowing users to easily provide their own decorators. Registered decorators would be collected and applied to DataSource by a BeanPostProcessor which could also support ordering decorators. Thoughts on this @philwebb?

@gavlyukovskiy
Copy link
Contributor Author

gavlyukovskiy commented Mar 28, 2017

Thanks for the answer, @philwebb, @vpavic.

@philwebb I tried to minimize breaking changes so my proposal is based on two conclusions:

  1. Keep actual type of data source bean is important, someone might be injecting explicit HikariDataSource. For that reason I have rejected approach with BeanPostProcessor which also a bit easier. Actually if data source get proxied user won't get the actual bean anyway, so using BeanPostProcessor will not break anything. Only one thing it is db call inside @PostConstruct will not be proxied.
  2. Settings done in data source specific configurations Dbcp, Dbcp2 and Tomcat also important.

@vpavic good point about DataSourceDecorator it removes the need of worst part - getting real data source using reflection. I tried to keep metrics working, with DataSourceDecorator it is easy to get real data source instance, also for user code that may rely on the real data source.

@gavlyukovskiy
Copy link
Contributor Author

gavlyukovskiy commented Mar 28, 2017

existing datasource beans that get exposed are bound to @ConfigurationProperties. If we return proxy variant of those beans then the properties won't get bound correctly.

are you talking about this part?

String validationQuery = databaseDriver.getValidationQuery();
if (validationQuery != null) {
	dataSource.setTestOnBorrow(true);
	dataSource.setValidationQuery(validationQuery);
}

In my case this will be executed if you use one of "default" proxies.

@vpavic
Copy link
Contributor

vpavic commented Mar 28, 2017

@gavlyukovskiy dataSource bean definitions in DataSourceConfiguration are annotated with @ConfigurationProperties which binds configuration properties directly to the appropriate dataSource bean.

See DataSourceConfiguration:53, DataSourceConfiguration:78 and DataSourceConfiguration:93.

@gavlyukovskiy
Copy link
Contributor Author

gavlyukovskiy commented Mar 28, 2017

I tried it again and now I see, very sorry for that dumb comments. Actually I thought that @ConfigurationProperties(prefix = "spring.datasource.hikari") means that DataSourceProperties passed into arguments will be taken from prefix spring.datasource.hikari, not from spring.datasource... But actaully these properties for the HikariDataSource and then ConfigurationPropertiesBindingPostProcessor injects these properties to the bean itself, now I see the full picture.

I'll try to use BeanPostProcessor and let's see how it will be.

@gavlyukovskiy gavlyukovskiy force-pushed the datasource-proxy-integration branch from e86c4d2 to 1db9dff Compare April 18, 2017 23:45
@gavlyukovskiy
Copy link
Contributor Author

I have changed the code to use decorators instead of direct proxy types. Also I'm using DecoratedDataSource to keep both real and decorated data sources for metadata.
BTW I have splited it out to a library - https://github.com/gavlyukovskiy/spring-boot-data-source-decorator and it works fine with existing spring-boot release.

@wilkinsona
Copy link
Member

Thanks for the PR and thanks, too, for the library that you've split out. Given that you're maintaining the library regularly and that it's included in our third-party starter list, I think that's the best place for this code now. If you disagree, please comment and we can reconsider.

@wilkinsona wilkinsona closed this Sep 29, 2017
@wilkinsona wilkinsona added status: declined A suggestion or change that we don't feel we should currently apply and removed status: on-hold We can't start working on this issue yet status: waiting-for-triage An issue we've not yet triaged labels Sep 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants