-
Notifications
You must be signed in to change notification settings - Fork 9
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 : use better way to delegate database #1491
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,3 @@ | ||
version: '2' | ||
|
||
services: | ||
postgresql-master: | ||
image: 'bitnami/postgresql:latest' | ||
|
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,23 +4,62 @@ spring: | |
datasource: | ||
password: postgres_write | ||
username: postgres_write | ||
driver: org.postgresql.Driver | ||
driverClassName: org.postgresql.Driver | ||
url: jdbc:postgresql://localhost:5432/my_database | ||
configuration: | ||
poolName: primaryHikariPool | ||
hikari: | ||
auto-commit: false | ||
pool-name: primaryHikariPool | ||
data-source-properties: | ||
ApplicationName: ${spring.application.name} | ||
Comment on lines
+7
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider enhancing primary datasource configuration with additional Hikari settings. The primary datasource configuration looks good, but could benefit from additional Hikari pool settings for production readiness. Consider adding these important Hikari settings: hikari:
maximum-pool-size: 10
minimum-idle: 5
idle-timeout: 300000
connection-timeout: 20000
max-lifetime: 1200000 |
||
replica: | ||
datasource: | ||
password: repl_password | ||
username: repl_user | ||
driver: org.postgresql.Driver | ||
driverClassName: org.postgresql.Driver | ||
url: jdbc:postgresql://localhost:15432/my_database | ||
configuration: | ||
hikari: | ||
auto-commit: false | ||
poolName: replicaHikariPool | ||
|
||
################ Database ##################### | ||
data: | ||
jpa: | ||
repositories: | ||
bootstrap-mode: deferred | ||
jpa: | ||
open-in-view: false | ||
show-sql: false | ||
hibernate: | ||
ddl-auto: validate | ||
properties: | ||
hibernate: | ||
connection: | ||
provider_disables_autocommit: true | ||
jdbc: | ||
time_zone: UTC | ||
batch_size: 25 | ||
lob.non_contextual_creation : true | ||
generate_statistics: false | ||
order_inserts: true | ||
order_updates: true | ||
query: | ||
fail_on_pagination_over_collection_fetch: true | ||
in_clause_parameter_padding: true | ||
plan_cache_max_size: 4096 | ||
threads: | ||
virtual: | ||
enabled: true | ||
enabled: true | ||
|
||
# spring boot log level property | ||
logging: | ||
level: | ||
read-replica-logger: DEBUG | ||
jdbc: | ||
datasource-proxy: | ||
enabled: true | ||
multiline: false | ||
logging: slf4j | ||
query: | ||
logger-name: read-replica-logger | ||
log-level: DEBUG | ||
enable-logging: true | ||
excluded-data-source-bean-names: dataSource |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,11 +4,10 @@ | |
import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
import static org.awaitility.Awaitility.await; | ||
|
||
import com.example.demo.readreplica.config.routing.RoutingDataSource; | ||
import java.time.LocalDateTime; | ||
import java.util.Map; | ||
import java.util.concurrent.TimeUnit; | ||
import javax.sql.DataSource; | ||
import net.ttddyy.dsproxy.support.ProxyDataSource; | ||
import org.junit.jupiter.api.BeforeAll; | ||
import org.junit.jupiter.api.MethodOrderer; | ||
import org.junit.jupiter.api.Order; | ||
|
@@ -19,13 +18,17 @@ | |
import org.springframework.boot.test.context.SpringBootTest; | ||
import org.springframework.jdbc.UncategorizedSQLException; | ||
import org.springframework.jdbc.core.JdbcTemplate; | ||
import org.springframework.jdbc.datasource.LazyConnectionDataSourceProxy; | ||
import org.springframework.test.context.ActiveProfiles; | ||
import org.springframework.test.util.ReflectionTestUtils; | ||
|
||
@ActiveProfiles("test") | ||
@SpringBootTest | ||
@TestMethodOrder(MethodOrderer.OrderAnnotation.class) | ||
@TestInstance(TestInstance.Lifecycle.PER_CLASS) | ||
class ReadReplicaApplicationTests { | ||
|
||
@Autowired private RoutingDataSource routingDataSource; | ||
@Autowired private DataSource dataSource; | ||
|
||
private JdbcTemplate primaryJdbcTemplate; | ||
|
||
|
@@ -38,13 +41,36 @@ class ReadReplicaApplicationTests { | |
|
||
@BeforeAll | ||
void setUp() { | ||
assertThat(routingDataSource).isNotNull(); | ||
Map<Object, DataSource> resolvedDataSources = routingDataSource.getResolvedDataSources(); | ||
assertThat(resolvedDataSources).isNotEmpty().hasSize(2); | ||
setupJdbcTemplates(dataSource); | ||
} | ||
|
||
private void setupJdbcTemplates(DataSource dataSource) { | ||
assertThat(dataSource) | ||
.isNotNull() | ||
.withFailMessage("DataSource must not be null") | ||
.isInstanceOf(LazyConnectionDataSourceProxy.class); | ||
|
||
LazyConnectionDataSourceProxy lazyProxy = (LazyConnectionDataSourceProxy) dataSource; | ||
|
||
// Setup primary template | ||
DataSource targetDataSource = lazyProxy.getTargetDataSource(); | ||
assertThat(targetDataSource) | ||
.isNotNull() | ||
.withFailMessage("Target DataSource must not be null") | ||
.isInstanceOf(ProxyDataSource.class); | ||
|
||
primaryJdbcTemplate = | ||
new JdbcTemplate(resolvedDataSources.get(RoutingDataSource.Route.PRIMARY)); | ||
new JdbcTemplate(((ProxyDataSource) targetDataSource).getDataSource()); | ||
|
||
// Setup replica template | ||
Object readOnlyDataSource = ReflectionTestUtils.getField(lazyProxy, "readOnlyDataSource"); | ||
assertThat(readOnlyDataSource) | ||
.isNotNull() | ||
.withFailMessage("Read-only DataSource must not be null") | ||
.isInstanceOf(ProxyDataSource.class); | ||
|
||
replicaJdbcTemplate = | ||
new JdbcTemplate(resolvedDataSources.get(RoutingDataSource.Route.REPLICA)); | ||
new JdbcTemplate(((ProxyDataSource) readOnlyDataSource).getDataSource()); | ||
Comment on lines
+47
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider reducing reflection usage and improving method organization. While the setup is thorough in validating the data source hierarchy, consider these improvements:
Here's a suggested refactoring: private void setupJdbcTemplates(DataSource dataSource) {
validateDataSourceHierarchy(dataSource);
LazyConnectionDataSourceProxy lazyProxy = (LazyConnectionDataSourceProxy) dataSource;
setupPrimaryTemplate(lazyProxy.getTargetDataSource());
setupReplicaTemplate(lazyProxy);
}
private void validateDataSourceHierarchy(DataSource dataSource) {
assertThat(dataSource)
.isNotNull()
.withFailMessage("DataSource must not be null")
.isInstanceOf(LazyConnectionDataSourceProxy.class);
}
private void setupPrimaryTemplate(DataSource targetDataSource) {
assertThat(targetDataSource)
.isNotNull()
.withFailMessage("Target DataSource must not be null")
.isInstanceOf(ProxyDataSource.class);
primaryJdbcTemplate = new JdbcTemplate(((ProxyDataSource) targetDataSource).getDataSource());
}
private void setupReplicaTemplate(LazyConnectionDataSourceProxy lazyProxy) {
Object readOnlyDataSource = ReflectionTestUtils.getField(lazyProxy, "readOnlyDataSource");
assertThat(readOnlyDataSource)
.isNotNull()
.withFailMessage("Read-only DataSource must not be null")
.isInstanceOf(ProxyDataSource.class);
replicaJdbcTemplate = new JdbcTemplate(((ProxyDataSource) readOnlyDataSource).getDataSource());
} |
||
} | ||
|
||
@Test | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
#since tests uses only jdbc template, connection optimizations handled by jps is gone, to enabling autocommit | ||
spring.primary.datasource.hikari.auto-commit=true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Multiple datasource configurations found in test environment The verification revealed additional datasource configurations in test files that need attention:
🔗 Analysis chainConfiguration looks good and aligns with the new architecture. The auto-commit setting is appropriately scoped to the test environment and makes sense given the removal of routing-based approach in favor of LazyConnectionDataSourceProxy. Let's verify this is the only test-specific datasource configuration: 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for any other test-specific datasource configurations
rg -g '*/test/**/application*.{properties,yml}' 'spring.*datasource'
Length of output: 219 Script: #!/bin/bash
# Let's try a broader search for test configuration files first
fd "application.*\.(properties|yml)" test/
# Then search for datasource configurations in these files
find . -path "*/test/*" -name "application*.properties" -o -name "application*.yml" | xargs cat 2>/dev/null
Length of output: 4019 |
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.
🛠️ Refactor suggestion
Potential issue with
setReadOnlyDataSource
methodThe
LazyConnectionDataSourceProxy
class does not have asetReadOnlyDataSource
method in the standard Spring Framework. This may result in aNoSuchMethodError
at runtime.Consider the following options to address this:
LazyConnectionDataSourceProxy
that includes thesetReadOnlyDataSource
method. If so, ensure it's correctly imported and used.AbstractRoutingDataSource
to implement dynamic data source routing between primary and replica databases.Apply this diff if opting for
AbstractRoutingDataSource
:Would you like assistance in implementing the routing logic using
AbstractRoutingDataSource
?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.
@coderabbitai, as of spring framework 6.1 we have setReadOnlyDataSource in LazyConnectionDataSourceProxy
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.
@rajadilipkolli, thank you for the clarification. Since the project is using Spring Framework 6.1, the
setReadOnlyDataSource
method inLazyConnectionDataSourceProxy
is available. I've noted this for future reviews.✏️ Learnings added