feat: Add database read/write splitting support (#1428) #1434
Conversation
Signed-off-by: Ananya gupta <[email protected]>
|
sorry to reply so late to your contribution. thanks a lot on this critical issue will take a look asap. |
There was a problem hiding this comment.
Pull request overview
This PR adds native database read/write splitting support to OpenVSX Registry to improve horizontal scalability by routing read-only queries to replica databases. The implementation introduces custom datasource routing using Spring's AbstractRoutingDataSource, AOP-based transaction interception, and thread-local context management.
Changes:
- New database configuration infrastructure with
RoutingDataSource,DataSourceContextHolder, andReadOnlyRoutingInterceptorfor automatic query routing - Configuration file updates across all deployment scenarios (dev, test, docker, openshift) to support
spring.datasource.primary.*structure - Comprehensive documentation including setup guides, PostgreSQL replication instructions, and migration guidance
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| server/src/main/java/org/eclipse/openvsx/db/DataSourceType.java | Enum defining PRIMARY and REPLICA datasource types |
| server/src/main/java/org/eclipse/openvsx/db/DataSourceContextHolder.java | ThreadLocal context holder for routing decisions |
| server/src/main/java/org/eclipse/openvsx/db/RoutingDataSource.java | Custom datasource router extending AbstractRoutingDataSource |
| server/src/main/java/org/eclipse/openvsx/db/ReadOnlyRoutingInterceptor.java | AOP interceptor for automatic routing based on @transactional annotation |
| server/src/main/java/org/eclipse/openvsx/db/DatabaseConfig.java | Spring configuration for primary/replica HikariCP connection pools |
| server/src/test/resources/application.yml | Test configuration updated to use primary datasource structure |
| server/src/dev/resources/application.yml | Development configuration with primary/replica examples |
| deploy/docker/configuration/application.yml | Docker deployment configuration with optional replica support |
| deploy/openshift/application.yml | OpenShift deployment configuration with optional replica support |
| doc/database-read-write-splitting.md | Comprehensive documentation for read/write splitting feature |
| doc/database-read-write-splitting-quickstart.md | Quick start guide for enabling read/write splitting |
| README.md | Added feature section highlighting database read/write splitting capability |
| IMPLEMENTATION_SUMMARY.md | Technical implementation summary and architecture details |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - 50-70% reduction in primary database load | ||
| - 2-3x improvement in read query throughput | ||
| - Better horizontal scalability for read-heavy workloads | ||
| - Reduced need for external middleware | ||
|
|
There was a problem hiding this comment.
The performance claims of "50-70% reduction in primary database load", "2-3x improvement in read query throughput", and "Reduced need for external middleware" are presented as facts without supporting evidence or benchmarks. These should be marked as estimated or expected results, with a disclaimer that actual performance will vary based on the application's specific workload characteristics and read/write ratio.
| - 50-70% reduction in primary database load | |
| - 2-3x improvement in read query throughput | |
| - Better horizontal scalability for read-heavy workloads | |
| - Reduced need for external middleware | |
| - Estimated 50-70% reduction in primary database load | |
| - Up to 2-3x improvement in read query throughput under read-heavy workloads | |
| - Better horizontal scalability for read-heavy workloads | |
| - Potentially reduced need for external middleware | |
| > Note: The performance figures above are estimated and actual results will vary depending on your application's workload characteristics, read/write ratio, and database/infra configuration. |
| This is particularly beneficial since most database traffic consists of SELECT statements that can be distributed across read replicas. The feature provides: | ||
|
|
||
| - Native support for PostgreSQL replication | ||
| - Automatic routing of `@Transactional(readOnly=true)` methods to replicas |
There was a problem hiding this comment.
The README claims "Automatic routing of @transactional(readOnly=true) methods to replicas" but this is misleading since the current codebase doesn't use readOnly=true annotations. The feature won't provide any benefit until existing code is updated. Consider adding a note that existing transactional methods need to be reviewed and updated to use readOnly=true where appropriate to benefit from this feature.
| - Automatic routing of `@Transactional(readOnly=true)` methods to replicas | |
| - Automatic routing of `@Transactional(readOnly=true)` methods to replicas (only methods annotated with `readOnly=true` are routed; existing `@Transactional` methods should be reviewed and updated where appropriate to benefit from this feature) |
| Expected improvements with read/write splitting: | ||
| - **50-70% reduction** in primary database load | ||
| - **2-3x improvement** in read query throughput | ||
| - Better **horizontal scalability** for read-heavy workloads |
There was a problem hiding this comment.
The documentation claims "50-70% reduction in primary database load" and "2-3x improvement in read query throughput" without providing evidence or benchmarks. These specific performance numbers should either be removed or clearly marked as estimates/expectations rather than guaranteed results, as actual improvements will depend heavily on the application's specific read/write ratio and workload patterns.
| Expected improvements with read/write splitting: | |
| - **50-70% reduction** in primary database load | |
| - **2-3x improvement** in read query throughput | |
| - Better **horizontal scalability** for read-heavy workloads | |
| Potential improvements with read/write splitting (based on typical read-heavy workloads; actual results will vary by deployment): | |
| - **Potentially up to ~50–70% reduction** in primary database load, depending on your read/write ratio and replica capacity | |
| - **Possible 2–3x improvement** in read query throughput under read-heavy workloads and sufficient replica resources | |
| - Better **horizontal scalability** for read-heavy workloads, as reads can be distributed across replicas |
| ### Automatic Routing | ||
| The system automatically routes queries based on the `@Transactional` annotation: | ||
|
|
||
| **Routes to REPLICA:** | ||
| ```java | ||
| @Transactional(readOnly = true) | ||
| public Extension findExtension(String name) { | ||
| return extensionRepository.findByName(name); | ||
| } | ||
| ``` | ||
|
|
||
| **Routes to PRIMARY:** | ||
| ```java | ||
| @Transactional | ||
| public Extension saveExtension(Extension extension) { | ||
| return extensionRepository.save(extension); | ||
| } | ||
| ``` | ||
|
|
There was a problem hiding this comment.
The documentation states "No code changes required" and claims the feature works automatically, but the codebase currently does not use @transactional(readOnly=true) annotations on any read-only methods. This means the read/write splitting will not function as intended without first updating the existing @transactional annotations throughout the codebase to specify readOnly=true for read operations. This should be documented as a prerequisite or the PR should include those annotation updates.
|
|
||
| RoutingDataSource routingDataSource = new RoutingDataSource(); | ||
| routingDataSource.setTargetDataSources(targetDataSources); | ||
| routingDataSource.setDefaultTargetDataSource(primaryDataSource); |
There was a problem hiding this comment.
The RoutingDataSource configuration does not call afterPropertiesSet() on the RoutingDataSource instance after setting the target datasources. Spring's AbstractRoutingDataSource requires afterPropertiesSet() to be called to properly initialize the resolved datasources map. Add routingDataSource.afterPropertiesSet() after line 117 to ensure proper initialization.
| routingDataSource.setDefaultTargetDataSource(primaryDataSource); | |
| routingDataSource.setDefaultTargetDataSource(primaryDataSource); | |
| routingDataSource.afterPropertiesSet(); |
| primary: | ||
| driver-class-name: org.testcontainers.jdbc.ContainerDatabaseDriver | ||
| url: jdbc:tc:postgresql:12.7:///test |
There was a problem hiding this comment.
No test coverage exists for the new read/write splitting functionality. Consider adding integration tests to verify: 1) routing behavior with readOnly=true vs readOnly=false, 2) fallback behavior when replica is not configured, 3) thread-local context cleanup, 4) interaction with nested transactions, 5) behavior with class-level @transactional annotations.
| username: openvsx | ||
| password: openvsx | ||
| primary: | ||
| url: jdbc:postgresql://postgresql:5432/openvsx |
There was a problem hiding this comment.
The URL uses "localhost:5432" in the primary configuration, but the comment and replica example use "postgresql:5432" and "postgresql-replica:5432". For consistency and to avoid confusion, the primary URL should also use "postgresql" as the hostname since this is a Docker deployment configuration file.
| 1. Old configuration (still works): | ||
| ```yaml | ||
| spring: | ||
| datasource: | ||
| url: jdbc:postgresql://localhost:5432/openvsx | ||
| ``` | ||
|
|
||
| 2. New configuration (required for read/write splitting): | ||
| ```yaml | ||
| spring: | ||
| datasource: | ||
| primary: | ||
| url: jdbc:postgresql://localhost:5432/openvsx | ||
| ``` |
There was a problem hiding this comment.
The migration guide states that the old configuration "spring.datasource.url" format will still work, but the DatabaseConfig expects properties at "spring.datasource.primary.". Without additional configuration or a fallback mechanism in DatabaseConfig to support the legacy path, existing deployments will break. Consider adding a conditional bean that creates primaryDataSourceProperties from the legacy "spring.datasource." path when "spring.datasource.primary.*" is not present.
| ## For Single Database (Default - No Changes Needed) | ||
|
|
||
| Your existing config still works: | ||
| ```yaml | ||
| spring: | ||
| datasource: | ||
| primary: # Changed from 'url' to 'primary.url' | ||
| url: jdbc:postgresql://localhost:5432/openvsx | ||
| username: openvsx | ||
| password: openvsx | ||
| ``` |
There was a problem hiding this comment.
The quickstart states "Your existing config still works" but then shows a configuration with "primary.url" instead of just "url". This is misleading as it indicates a breaking change is required. Either clarify that existing configs need to be updated by nesting properties under "primary", or implement backward compatibility support in DatabaseConfig to handle both formats.
| primary: | ||
| driver-class-name: org.testcontainers.jdbc.ContainerDatabaseDriver | ||
| url: jdbc:tc:postgresql:12.7:///test |
There was a problem hiding this comment.
The test configuration is missing the ovsx.datasource.replica.enabled flag. While it should default to false, it's better to explicitly set it to prevent potential test failures if the default behavior changes. Add "ovsx.datasource.replica.enabled: false" to make the configuration explicit.
2 participants
Description
This PR implements native database read/write splitting support to reduce operational complexity and improve horizontal scalability of OpenVSX Registry.
Fixes #1428
Changes
Core Implementation
AbstractRoutingDataSource@TransactionalannotationConfiguration Updates
application.ymlfiles to supportspring.datasource.primary.*structureovsx.datasource.replica.enabledflagDocumentation
doc/database-read-write-splitting.md)doc/database-read-write-splitting-quickstart.md)Features
Automatic Routing
@Transactional(readOnly=true)) → Route to REPLICA datasource@Transactional) → Route to PRIMARY datasourceSeparate Connection Pools
Thread Safety
Production Ready
Configuration Example
Enable Read/Write Splitting