-
Notifications
You must be signed in to change notification settings - Fork 40
Support begin in read-only mode for JDBC transactions #2738
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
Support begin in read-only mode for JDBC transactions #2738
Conversation
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.
Pull Request Overview
This PR adds support for starting JDBC transactions in read-only mode and updates related tests and execution logic.
- Introduces
beginReadOnlyoverloads and a unifiedbegin(txId, readOnly)implementation - Updates
executeTransactionto distinguish between read-only and read-write operations - Adjusts unit tests to cover read-only behavior and cleans up integration tests
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| core/src/test/java/com/scalar/db/transaction/jdbc/JdbcTransactionManagerTest.java | Added tests for beginReadOnly variants and updated scanner tests to use read-only transactions |
| core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransactionManager.java | Added beginReadOnly methods, unified begin(txId, readOnly), and updated executeTransaction to accept a read-only flag |
| core/src/integration-test/java/com/scalar/db/transaction/jdbc/JdbcTransactionIntegrationTest.java | Removed placeholder disabled tests for read-only integration scenarios |
Comments suppressed due to low confidence (4)
core/src/integration-test/java/com/scalar/db/transaction/jdbc/JdbcTransactionIntegrationTest.java:42
- Integration tests for read-only transactions have been removed; consider implementing and re-enabling tests to validate read-only begin methods in real database scenarios.
@Disabled("Implement later")
core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransactionManager.java:104
- Public methods
beginReadOnlyandbeginReadOnly(String)lack Javadoc comments; consider adding descriptions for their behavior and parameters.
public DistributedTransaction beginReadOnly() throws TransactionException {
core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransactionManager.java:114
- [nitpick] Consider renaming the boolean parameter
readOnlytoisReadOnlyfor clearer intent in the method signature.
private DistributedTransaction begin(String txId, boolean readOnly) throws TransactionException {
core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransactionManager.java:376
- [nitpick] The
executeTransactionmethod now requires an explicit readOnly flag for each call; consider providing an overload without the flag (defaulting to write mode) to simplify calls for common write operations.
private <R> R executeTransaction(
|
|
||
| DistributedTransaction transaction; | ||
| if (readOnly) { | ||
| rdbEngine.setReadOnly(connection, true); |
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.
Call Connection.setReadOnly() for read-only transactions.
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.
I noticed the method name setReadOnly() seems to change the rdbEngine to read-only mode. setConnectionToReadOnly() might be less confusing. What do you think?
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.
Fixed in 60c000f. Thanks!
| transaction = | ||
| new ReadOnlyDistributedTransaction( | ||
| new JdbcTransaction(txId, jdbcService, connection, rdbEngine)); |
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.
Wrap the transaction object with ReadOnlyDistributedTransaction.
komamitsu
left a comment
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.
LGTM! 👍
feeblefakie
left a comment
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.
LGTM! Thank you!
Torch3333
left a comment
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.
LGTM, thank you!
Description
This PR adds support for beginning a transaction in read-only mode for JDBC transactions.
Note that this feature is being developed in the
support-begin-in-read-only-modefeature branch.Related issues and/or PRs
Changes made
Added some inline comments. Please take a look for the details!
Checklist
Additional notes (optional)
N/A
Release notes
N/A