feat: Add lazy refresh strategy to the connector. Fixes #992. #1990
Conversation
core/src/main/java/com/google/cloud/sql/core/DefaultConnectionInfoCache.java
Outdated
Show resolved
Hide resolved
| MIN_REFERSH_DELAY_MS); | ||
|
|
||
| ConnectionMetadata gotMetadata = connectionInfoCache.getConnectionMetadata(TEST_TIMEOUT_MS); | ||
| assertThat(gotMetadata.getKeyManagerFactory()) |
There was a problem hiding this comment.
What is the significance of this assertion? Would the counter check be enough for an assertion?
There was a problem hiding this comment.
Fixed. I added comments to explain.
| AuthType authType, | ||
| KeyPair keyPair) { | ||
| try { | ||
| Thread.sleep(100); |
There was a problem hiding this comment.
The test needs to simulate a blocking call.
hessjcg
force-pushed
the
gh-992-implement-lazy-strategy
branch
from
1abf29b
to
84967d7
Compare
hessjcg
force-pushed
the
gh-992-implement-lazy-strategy
branch
from
84967d7
to
0c74936
Compare
| } catch (InterruptedException | ExecutionException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| return new DefaultConnectionInfoCache( |
There was a problem hiding this comment.
Looking more closely at this, I see we're missing a ConnectionInfoCache interface in this repo (which exists in Go and in AlloyDB Java). Instead of having multiple constructors for one cache type, we should be introducing separate caches that both implement a ConnectionInfoCache interface. This makes for better separation of concerns. The refresher strategy might still be useful, but the most important thing it to think of connection info cache as being the interface for which we have multiple implementations.
hessjcg
force-pushed
the
gh-992-implement-lazy-strategy
branch
from
0c74936
to
a42f60c
Compare
hessjcg
force-pushed
the
gh-992-implement-lazy-strategy
branch
from
a42f60c
to
f8091ae
Compare
hessjcg
force-pushed
the
gh-992-implement-lazy-strategy
branch
from
f8091ae
to
31e7eab
Compare
hessjcg
force-pushed
the
gh-992-refactor-refresh-ahead-cache
branch
4 times, most recently
from
0ca91ff
to
6143994
Compare
hessjcg
force-pushed
the
gh-992-implement-lazy-strategy
branch
from
31e7eab
to
155809a
Compare
|
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
hessjcg
force-pushed
the
gh-992-refactor-refresh-ahead-cache
branch
from
6143994
to
84ef897
Compare
hessjcg
force-pushed
the
gh-992-implement-lazy-strategy
branch
2 times, most recently
from
496b4f1
to
ccb7678
Compare
hessjcg
force-pushed
the
gh-992-refactor-refresh-ahead-cache
branch
from
84ef897
to
86a443e
Compare
hessjcg
force-pushed
the
gh-992-implement-lazy-strategy
branch
from
ccb7678
to
cd26691
Compare
hessjcg
force-pushed
the
gh-992-refactor-refresh-ahead-cache
branch
from
86a443e
to
f7de7c0
Compare
hessjcg
force-pushed
the
gh-992-implement-lazy-strategy
branch
from
cd26691
to
9518af3
Compare
hessjcg
force-pushed
the
gh-992-implement-lazy-strategy
branch
2 times, most recently
from
10b3cf1
to
e5d7f08
Compare
|
Refactoring is complete. Please give this Lazy Refresh implementation another look. |
| ConnectionMetadata gotMetadata = connectionInfoCache.getConnectionMetadata(300); | ||
|
|
||
| // Assert that the underlying ConnectionInfo was retrieved exactly once. | ||
| assertThat(instanceDataSupplier.counter.get()).isEqualTo(1); |
There was a problem hiding this comment.
Let's call the method above twice to prove we're calling the underlying instance data supplier only once.
hessjcg
force-pushed
the
gh-992-implement-lazy-strategy
branch
from
e5d7f08
to
fa7848e
Compare
The lazy refresh strategy only refreshes credentials and certificate information when the application attempts
to establish a new database connection. On Cloud Run and other serverless runtimes, this is more reliable
than the default background refresh strategy.
Fixes #992.