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.
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
Introduce EnvironmentPostProcessor for R2DBC support in Cloud Sql #772
Introduce EnvironmentPostProcessor for R2DBC support in Cloud Sql #772
Changes from 14 commits
31d7ed8
7711250
92ca251
eeacab9
37ee37c
92b23e0
139fb92
99202b3
dee8706
5796bdf
8da6245
2975a77
2394ad6
a428256
8bd52bf
ac2334b
c8497b8
1fdf4c8
a494df6
c05e025
1171bec
e9bab44
29d1540
c44ca56
31e7ddf
cf7d15c
f4b758d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It's a minor thing, but I think there is even more code here that can be shared by adding some extra parameters to a helper method. It could make sense, unless it makes the code less readable.
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.
Hm that's a good point. One potential area I can think of refactoring is
CloudJdbcInfoProvider
andDefaultCloudSqlJdbcProvider
. We used this interface in the past because we also hadAppEngineCloudSqlJdbcInfoProvider
but we may not need this interface anymore -- this can help reduce added hierarchy in the code. Additionally some of the code that is used to create a url can be shared between the r2dbc and jdbc workflow. I can do this refactoring in a follow-up PR if you think this is a good idea.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.
Make sure that R2dbc post-processing is not triggered if the user just wants plain old JDBC>
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.
Done. This method now returns
null
if the dependency that is used to enable Spring R2DBC auto-configuration is not present.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.
What if the user has the R2DBC dependency, but doesn't want our auto-configuration to kick in? I think we might need something like
spring.cloud.gcp.sql.r2dbc.enabled
.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.
That would be instead of
spring.cloud.gcp.sql.enabled
for Cloud SQL R2DBC autoconfig being on, not in addition to, right?I guess theoretically it's possible that an app wants
spring.cloud.gcp.sql.enabled
for blocking connection to Cloud SQL, and then an R2DBC driver to another type of database. It would be weird for a reactive app to have blocking components, but not impossible.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.
Ideally,
spring.cloud.gcp.sql.enabled=false
would disable all of our Cloud SQL starters.Maybe we need to add two properties:
spring.cloud.gcp.sql.r2dbc.enabled
andspring.cloud.gcp.sql.jdbc.enabled
.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.
So for this PR, the check would be that both
spring.cloud.gcp.sql.enabled
andspring.cloud.gcp.sql.r2dbc.enabled
are true. That sounds good.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.
Yep. Also, consider introducing
spring.cloud.gcp.sql.jdbc.enabled
.Eventually, we might want to deprecate
spring.cloud.gcp.sql.enabled
.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.
This is something that I did not think of before. Since it is possible for an app to expect a blocking connection while having the r2dbc driver on the classpath, introducing
spring.cloud.gcp.sql.r2dbc.enabled
sounds like a good idea. I'll addspring.cloud.gcp.sql.jdbc.enabled
in a separate PR since it might break existing behavior for current cloud sql customers.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.
@meltsufin @elefeint Added
spring.cloud.gcp.sql.r2dbc.enabled
which will betrue
by default.