-
Notifications
You must be signed in to change notification settings - Fork 40
Support transaction metadata decoupling in Consensus Commit #3207
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 transaction metadata decoupling in Consensus Commit #3207
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 significantly enhances Consensus Commit by introducing transaction metadata decoupling. This feature allows for a more flexible and potentially optimized storage of transaction metadata by separating it from the main data. The implementation involves substantial changes to how tables are created and imported, leveraging virtual tables to join data and metadata. Robust validation has been added to ensure operations on these new table structures are consistent and supported by the underlying storage. The changes are thoroughly tested with new integration tests covering various database types and schema loading scenarios. 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 support for transaction metadata decoupling in Consensus Commit, a significant feature that separates transaction metadata from application data. The changes are extensive, touching core logic and adding a substantial number of integration tests. The implementation appears solid, but there are several areas with code duplication, particularly in the test setup, that could be refactored to improve maintainability. I've provided specific suggestions for these refactorings.
...ge/jdbc/ConsensusCommitImportTableWithMetadataDecouplingIntegrationTestWithJdbcDatabase.java
Outdated
Show resolved
Hide resolved
.../com/scalar/db/storage/jdbc/JdbcSchemaLoaderImportWithMetadataDecouplingIntegrationTest.java
Show resolved
Hide resolved
...t/java/com/scalar/db/storage/jdbc/JdbcSchemaLoaderWithMetadataDecouplingIntegrationTest.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/storage/jdbc/JdbcDatabase.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdmin.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 introduces support for transaction metadata decoupling in Consensus Commit, enabling storage of transaction metadata in separate tables from user data. This is a significant architectural enhancement that improves performance and scalability for certain storage backends.
Key Changes:
- Added transaction metadata decoupling feature with separate data and metadata tables joined via virtual tables
- Refactored
SchemaOperator.importTables()to retrieve options from eachImportTableSchemarather than passing a global options parameter - Extended integration test suites with new base classes for metadata decoupling scenarios
- Added validation to ensure metadata decoupling is only used with supported storage backends (JDBC with proper isolation levels)
Reviewed Changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
ConsensusCommitAdmin.java |
Implements create/drop/import table operations for metadata decoupling with virtual tables |
ConsensusCommitUtils.java |
Adds utility to build transaction metadata table schema from data table metadata |
ConsensusCommitOperationChecker.java |
Validates operations on virtual tables require consistent reads from storage |
SchemaOperator.java |
Refactored to use per-table options from ImportTableSchema.getOptions() |
JdbcDatabase.java |
Handles LEFT_OUTER join conditions for virtual tables with IS_NULL checks |
JdbcUtils.java |
Configures transaction isolation levels for JDBC data sources |
| Integration test bases | New test base classes for metadata decoupling scenarios across different storage backends |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.../com/scalar/db/storage/jdbc/JdbcSchemaLoaderImportWithMetadataDecouplingIntegrationTest.java
Show resolved
Hide resolved
.../com/scalar/db/storage/jdbc/JdbcSchemaLoaderImportWithMetadataDecouplingIntegrationTest.java
Show resolved
Hide resolved
.../com/scalar/db/storage/jdbc/JdbcSchemaLoaderImportWithMetadataDecouplingIntegrationTest.java
Show resolved
Hide resolved
a93e918 to
629633b
Compare
5363a25 to
b82c7b3
Compare
b82c7b3 to
a7f4cd8
Compare
|
/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 pull request introduces transaction metadata decoupling, a significant feature that allows ScalarDB to work with existing data without migration. The implementation looks solid, with new logic in ConsensusCommitAdmin to handle the creation, import, and dropping of decoupled tables. The changes are well-supported by a comprehensive set of new integration tests.
I have two main points of feedback:
- The behavior of
dropTablefor imported tables is destructive as it drops the user's original data table. This should be reconsidered to avoid unexpected data loss. - There is some duplicated code in the new test classes for setting the JDBC isolation level, which could be refactored for better maintainability.
Overall, this is a great enhancement to ScalarDB.
core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdmin.java
Show resolved
Hide resolved
...bc/ConsensusCommitAdminImportTableWithMetadataDecouplingIntegrationTestWithJdbcDatabase.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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
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.
Overall, looking good. Thank you!
Left some minor comments. PTAL!
core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdmin.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdmin.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdmin.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!
core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitAdmin.java
Show resolved
Hide resolved
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!
…-consensus-commit
…-consensus-commit
Description
This PR adds support for transaction metadata decoupling in Consensus Commit. This feature allows transaction metadata to be stored separately from user data, enabling the use of virtual tables that reference existing database tables without copying data.
Implementation details
Transaction metadata decoupling enabled when
transaction_metadata_decoupling=true is specified in optionsConsensusCommitAdmin.createTable()with transaction metadata decoupling<table_name><table_name>_data<table_name>_tx_metadatathrowIfTransactionMetadataDecouplingNotSupportedStorage()NAMESPACEorSTORAGElevel atomicity)<table_name>_data, stores only user data<table_name>_tx_metadata, contains:tx_id,tx_state,tx_version, before columns, etc.)<table_name>, joins data table and metadata table withINNER JOINConsensusCommitAdmin.importTable()with transaction metadata decoupling<table_name>(used as data table)<table_name>_tx_metadata<table_name>_scalardbthrowIfTransactionMetadataDecouplingNotSupportedStorage()<table_name>_tx_metadata<table_name>_scalardb, joins data and metadata tables withLEFT OUTER JOINLEFT OUTER JOINallows reading records not yet processed by transactions (no metadata exists yet)ConsensusCommitAdmin.dropTable()Unsupported administrative operations:
Added checks for all CRID operations in
ConsensusCommitOperationCheckerCurrently, to determine whether a table is a transaction metadata–decoupling table, we simply check whether it is a virtual table. In the future, we may need to introduce dedicated metadata for this in Consensus Commit.
Related issues and/or PRs
Changes made
Core Functionality
ConsensusCommitAdmin: Added support for creating and managing virtual tables with metadata decouplingConsensusCommitUtils: Added utility methods for virtual table operationsConsensusCommitOperationChecker: Added validation logic to prevent unsupported operations on virtual tables (e.g., mutations on read-only virtual tables)Schema Loader
Added comprehensive integration tests for metadata decoupling:
ConsensusCommitWithMetadataDecouplingIntegrationTestBaseConsensusCommitImportTableWithMetadataDecouplingIntegrationTestBaseConsensusCommitAdminImportTableWithMetadataDecouplingIntegrationTestBaseConsensusCommitSpecificWithMetadataDecouplingIntegrationTestBaseSchemaLoaderWithMetadataDecouplingIntegrationTestBaseSchemaLoaderImportWithMetadataDecouplingIntegrationTestBaseChecklist
Additional notes (optional)
N/A
Release notes
Added transaction metadata decoupling support in Consensus Commit. This feature enables users to perform Consensus Commit ScalarDB transactions on pre-existing data without schema modifications or data migration.