Add beginReadOnly() method to transaction abstraction #2734
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR introduces the beginReadOnly() method to the transaction abstraction along with supporting tests and service API updates. Key changes include:
- Adding read-only transaction methods in various transaction managers and the TransactionService API.
- Implementing read-only transaction behavior by raising UnsupportedOperationExceptions (or placeholders for future work) in managers that do not yet support read-only transactions.
- Updating integration tests to cover and disable read-only transaction scenarios.
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| integration-test/src/main/java/com/scalar/db/transaction/singlecrudoperation/SingleCrudOperationTransactionIntegrationTestBase.java | Added disabled tests for read-only transactions in single CRUD operations. |
| integration-test/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitIntegrationTestBase.java | Added disabled read-only tests for consensus commit transactions. |
| integration-test/src/main/java/com/scalar/db/api/DistributedTransactionIntegrationTestBase.java | Added enabled tests for read-only transactions in distributed transactions. |
| core/src/main/java/com/scalar/db/transaction/singlecrudoperation/SingleCrudOperationTransactionManager.java | Implemented read-only methods that throw an unsupported exception. |
| core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransactionManager.java | Introduced placeholder read-only methods with "implement later" messages. |
| core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitManager.java | Introduced placeholder read-only methods with "implement later" messages. |
| core/src/main/java/com/scalar/db/service/TransactionService.java | Delegated beginning of read-only transactions to the manager. |
| core/src/main/java/com/scalar/db/common/error/CoreError.java | Added a new error code for mutation operations in read-only transactions. |
| Other files | Added annotations and API documentation updates to support the new read-only transaction methods. |
|
|
||
| @Override | ||
| public DistributedTransaction beginReadOnly() { | ||
| throw new UnsupportedOperationException("implement later"); |
There was a problem hiding this comment.
Consider replacing the placeholder error message with a more descriptive and consistent message (or a reference to a constant error code) similar to the one used in the SingleCrudOperationTransactionManager.
core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitManager.java
Show resolved
Hide resolved
brfrn169
commented
Jun 4, 2025
| * faults. You can try retrying the transaction, but you may not be able to begin the | ||
| * transaction due to nontransient faults | ||
| */ | ||
| DistributedTransaction beginReadOnly() throws TransactionNotFoundException, TransactionException; |
Collaborator
Author
There was a problem hiding this comment.
Added several methods for the read-only mode.
| import javax.annotation.concurrent.NotThreadSafe; | ||
|
|
||
| @NotThreadSafe | ||
| public class ReadOnlyDistributedTransaction extends DecoratedDistributedTransaction { |
Collaborator
Author
There was a problem hiding this comment.
Added a wrapper class to enforce immutability for read-only transactions.
Comment on lines
+148
to
+156
| @Override | ||
| public DistributedTransaction beginReadOnly() { | ||
| throw new UnsupportedOperationException("implement later"); | ||
| } | ||
|
|
||
| @Override | ||
| public DistributedTransaction beginReadOnly(String txId) { | ||
| throw new UnsupportedOperationException("implement later"); | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
I will add an implementation for Consensus Commit in a separate PR after this PR is merged.
Comment on lines
+112
to
+120
| @Override | ||
| public DistributedTransaction beginReadOnly() { | ||
| throw new UnsupportedOperationException("implement later"); | ||
| } | ||
|
|
||
| @Override | ||
| public DistributedTransaction beginReadOnly(String txId) { | ||
| throw new UnsupportedOperationException("implement later"); | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
I will add an implementation for JDBC transactions in a separate PR after this PR is merged.
Comment on lines
+79
to
+91
| @Override | ||
| public DistributedTransaction beginReadOnly() throws TransactionException { | ||
| throw new UnsupportedOperationException( | ||
| CoreError.SINGLE_CRUD_OPERATION_TRANSACTION_BEGINNING_TRANSACTION_NOT_ALLOWED | ||
| .buildMessage()); | ||
| } | ||
|
|
||
| @Override | ||
| public DistributedTransaction beginReadOnly(String txId) throws TransactionException { | ||
| throw new UnsupportedOperationException( | ||
| CoreError.SINGLE_CRUD_OPERATION_TRANSACTION_BEGINNING_TRANSACTION_NOT_ALLOWED | ||
| .buildMessage()); | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
For the Single Crud Operation transaction manager, just throw UnsupportedOperationException, since the Single Crud Operation transaction manager doesn't support beginning transactions.
This was referenced Jun 5, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Description
This PR adds a
beginReadOnly()method toDistributedTransactionManagerto begin a transaction in read-only mode.We do not add this method to
TwoPhaseCommitTransactionManagerbecause ensuring and managing read-only behavior across multiple transaction managers is challenging.Please note that this feature is being developed in the
support-begin-in-read-only-modefeature branch, as the changes may cause compile errors in dependent projects. Once all implementations are complete, we will merge this feature branch into themasterbranch.Related issues and/or PRs
N/A
Changes made
Added some inline comments. Please take a look for the details!
Checklist
Additional notes (optional)
N/A
Release notes
N/A