-
Notifications
You must be signed in to change notification settings - Fork 41
Add configuration option to enable/disable active transaction management #3233
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
Add configuration option to enable/disable active transaction management #3233
Conversation
Summary of ChangesHello @brfrn169, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new configuration option to explicitly enable or disable active transaction management, ensuring backward compatibility by setting the default to Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a configuration option to enable or disable active transaction management and refactors transaction provider implementations into a new abstract base class, AbstractDistributedTransactionProvider. The changes are well-structured, and the new configuration option is properly integrated and tested. The refactoring successfully centralizes the transaction manager decoration logic. I have one suggestion to address a potential issue in the new abstract class where a state management wrapper appears to be missing for two-phase commit transactions.
core/src/main/java/com/scalar/db/api/AbstractDistributedTransactionProvider.java
Show resolved
Hide resolved
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 a new configuration option scalar.db.active_transaction_management.enabled to control whether active transaction management is applied to transaction managers, with a default value of true for backward compatibility. The PR also refactors the transaction provider architecture by introducing AbstractDistributedTransactionProvider as a base class to consolidate the wrapper logic.
Key Changes:
- Added
scalar.db.active_transaction_management.enabledconfiguration property with default valuetrue - Introduced
AbstractDistributedTransactionProviderbase class to centralize transaction manager wrapping logic - Refactored
ConsensusCommitProviderandJdbcTransactionProviderto extend the new abstract class and implementcreateRaw*methods
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
core/src/main/java/com/scalar/db/config/DatabaseConfig.java |
Added configuration property and getter for enabling/disabling active transaction management |
core/src/main/java/com/scalar/db/api/AbstractDistributedTransactionProvider.java |
New abstract base class that conditionally applies state management and active transaction management wrappers based on configuration |
core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitProvider.java |
Refactored to extend AbstractDistributedTransactionProvider and implement raw transaction manager creation methods |
core/src/main/java/com/scalar/db/transaction/jdbc/JdbcTransactionProvider.java |
Refactored to extend AbstractDistributedTransactionProvider and implement raw transaction manager creation methods |
core/src/test/java/com/scalar/db/config/DatabaseConfigTest.java |
Added assertions to verify the new configuration property defaults to true and can be set to false |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
core/src/main/java/com/scalar/db/api/AbstractDistributedTransactionProvider.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/api/AbstractDistributedTransactionProvider.java
Show resolved
Hide resolved
…tiveTransactionManagedTwoPhaseCommitTransactionManager
|
/gemini review |
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.
Code Review
This is a great pull request that significantly improves the codebase. The introduction of AbstractDistributedTransactionProvider effectively reduces code duplication. The extraction of logic into ActiveTransactionRegistry and SynchronizedScanner makes the transaction management classes cleaner and more focused. The new configuration option to disable active transaction management is also a valuable addition. I have a couple of suggestions for further improvement.
.../java/com/scalar/db/common/ActiveTransactionManagedTwoPhaseCommitTransactionManagerTest.java
Outdated
Show resolved
Hide resolved
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
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
core/src/main/java/com/scalar/db/api/AbstractDistributedTransactionProvider.java
Show resolved
Hide resolved
.../java/com/scalar/db/common/ActiveTransactionManagedTwoPhaseCommitTransactionManagerTest.java
Outdated
Show resolved
Hide resolved
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!
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!
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! 👍
…-transaction-management
Description
This PR adds
scalar.db.active_transaction_management.enabledconfiguration option to enable/disable the active transaction management. The default value istruefor backward compatibility.Related issues and/or PRs
N/A
Changes made
scalar.db.active_transaction_management.enabledconfiguration option to enable/disable the active transaction management (default:true).AbstractDistributedTransactionProvideras a base class to consolidate wrapper logic for transaction managers.ConsensusCommitProviderandJdbcTransactionProviderto extend the new abstract class.ActiveTransactionManagedDistributedTransactionManagerandActiveTransactionManagedTwoPhaseCommitTransactionManager.ActiveTransactionRegistryand
SynchronizedScanner.Checklist
Additional notes (optional)
N/A
Release notes
Added
scalar.db.active_transaction_management.enabledconfiguration option to enable/disable the active transaction management (default:true).