-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: add support for lazy refresh #606
base: main
Are you sure you want to change the base?
Conversation
alloydb-jdbc-connector/src/main/java/com/google/cloud/alloydb/ConnectorConfig.java
Show resolved
Hide resolved
...jdbc-connector/src/main/java/com/google/cloud/alloydb/DefaultConnectionInfoCacheFactory.java
Show resolved
Hide resolved
@@ -30,7 +31,8 @@ public class AlloyDbJdbcNamedConnectorDataSourceFactory { | |||
static HikariDataSource createDataSource() { | |||
|
|||
// Register a named Connector | |||
ConnectorConfig namedConnectorConfig = new ConnectorConfig.Builder().build(); | |||
ConnectorConfig namedConnectorConfig = | |||
new ConnectorConfig.Builder().withRefreshStrategy(RefreshStrategy.LAZY).build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we changing this to use a lazy cache?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, it'd be interesting to know. I see this is only used in the ITPscTest.java
test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have one integration test covering lazy refresh. While we could introduce yet another integration test, there are already a handful of integration tests that aren't so different from each other. So rather than make our test suite a little slower with another duplicate test, I opted to just ensure one of our existing integration tests uses lazy refresh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some comments.
alloydb-jdbc-connector/src/main/java/com/google/cloud/alloydb/LazyConnectionInfoCache.java
Show resolved
Hide resolved
@@ -30,7 +31,8 @@ public class AlloyDbJdbcNamedConnectorDataSourceFactory { | |||
static HikariDataSource createDataSource() { | |||
|
|||
// Register a named Connector | |||
ConnectorConfig namedConnectorConfig = new ConnectorConfig.Builder().build(); | |||
ConnectorConfig namedConnectorConfig = | |||
new ConnectorConfig.Builder().withRefreshStrategy(RefreshStrategy.LAZY).build(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, it'd be interesting to know. I see this is only used in the ITPscTest.java
test
By default the connector will continue to use a refresh ahead strategy, where client certificates are refreshed by a background thread. The lazy strategy by comparison is useful for when the Connector runs in serverless environments and background threads may not run reliably, e.g., when the CPU is throttled. This commit adds the lazy refresh strategy and updates the configuration documentation to demonstrate how to use the feature. Fixes #565
By default the connector will continue to use a refresh ahead strategy, where client certificates are refreshed by a background thread. The lazy strategy by comparison is useful for when the Connector runs in serverless environments and background threads may not run reliably, e.g., when the CPU is throttled.
This commit adds the lazy refresh strategy and updates the configuration documentation to demonstrate how to use the feature.
Fixes #565