Improve internal connection pool key defaults #432
Merged
Conversation
This file contains 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
sergiyvamz
reviewed
May 5, 2023
wrapper/src/main/java/software/amazon/jdbc/HikariPooledConnectionProvider.java
Outdated
Show resolved
Hide resolved
sergiyvamz
reviewed
May 5, 2023
| private String getPoolKey(HostSpec hostSpec, Properties props) { | ||
| return poolMapping.getKey(hostSpec, props) | ||
| + props.getProperty(PropertyDefinition.USER.name) | ||
| + props.getProperty(PropertyDefinition.PASSWORD.name); |
There was a problem hiding this comment.
I wouldn't include password. In case of IAM token (that is changing every15min) we might see more and more connection pools are created.
sergiyvamz
reviewed
May 5, 2023
| @@ -758,4 +760,75 @@ public void test_pooledConnection_failoverInTransaction() | |||
| ConnectionProviderManager.resetProvider(); | |||
| } | |||
| } | |||
|
|
|||
| @TestTemplate | |||
| @EnableOnTestFeature(TestEnvironmentFeatures.FAILOVER_SUPPORTED) | |||
There was a problem hiding this comment.
This test practically test how well getPoolKey() works. I don't think we need such test as integration test with real database. A simple unit test with mocked connection pools would be more than enough.
joyc-bq
approved these changes
May 8, 2023
karenc-bq
reviewed
May 9, 2023
sergiyvamz
approved these changes
May 9, 2023
karenc-bq
reviewed
May 9, 2023
| @@ -39,7 +39,7 @@ The wrapper driver currently uses [Hikari](https://github.com/brettwooldridge/Hi | |||
| - username | |||
| - password | |||
|
|
|||
| You can optionally pass in a `HikariPoolMapping` function as a second parameter to the `HikariPooledConnectionProvider`. Internally, the connection pools used by the plugin are maintained as a map from instance URLs to connection pools. If you would like to define a different key system, you should pass in a `HikariPoolMapping` function defining this logic. This is helpful, for example, when you would like to create multiple Connection objects to the same instance with different users. In this scenario, you should pass in a `HikariPoolMapping` that incorporates the instance URL and the username from the `Properties` object into the map key. | |||
| You can optionally pass in a `HikariPoolMapping` function as a second parameter to the `HikariPooledConnectionProvider`. This allows you to decide when new connection pools should be created by defining what is included in the pool map key. A new pool will be created each time a new connection is requested with a unique key. By default, a new pool will be created for each unique instance-user combination. If you would like to define a different key system, you should pass in a `HikariPoolMapping` function defining this logic. Note that the user will always be automatically included in the key for security reasons. | |||
There was a problem hiding this comment.
Can we provide an example?
karenc-bq
approved these changes
May 10, 2023
karenc-bq
approved these changes
May 10, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.