-
Notifications
You must be signed in to change notification settings - Fork 40
Add isConsistentVirtualTableReadGuaranteed() to StorageInfo #3204
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 isConsistentVirtualTableReadGuaranteed() to StorageInfo #3204
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 enhances the 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 PR introduces a new method isConsistentVirtualTableRead() to the StorageInfo interface. The implementation looks good, especially the dynamic check for JDBC storages. I've found a few issues, including a potential bug in StorageInfoImpl where equals() and hashCode() are not updated, and an incorrect isolation level for SQLite. I've also suggested a refactoring for the new tests in JdbcAdminTest to reduce duplication.
Also, please note there is an inconsistency in the method name between the PR description/release notes (isConsistentReadGuaranteed) and the actual implementation (isConsistentVirtualTableRead). It would be good to make them consistent.
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 isConsistentVirtualTableRead() method to the StorageInfo interface to indicate whether a storage backend guarantees consistent reads for virtual tables. The implementation is storage-specific: most storage backends (Cassandra, Cosmos DB, DynamoDB, Object Storage) return false as they don't support virtual tables, while JDBC implementations dynamically determine the value based on the database's transaction isolation level.
Key changes:
- Added
isConsistentVirtualTableRead()method toStorageInfointerface with appropriate documentation - Implemented the method in
StorageInfoImplwith a new boolean field - Updated all storage implementations (Cassandra, Cosmos, DynamoDB, Object Storage, JDBC, MultiStorage) to provide the new parameter
- Added database-specific logic in each
RdbEngineimplementation to determine minimum isolation levels for consistent reads
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
core/src/main/java/com/scalar/db/api/StorageInfo.java |
Added new isConsistentVirtualTableRead() method to interface |
core/src/main/java/com/scalar/db/api/DistributedStorageAdmin.java |
Added documentation note that getStorageInfo is for internal use |
core/src/main/java/com/scalar/db/common/StorageInfoImpl.java |
Added new boolean field and getter implementation |
core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineStrategy.java |
Added getMinimumIsolationLevelForConsistencyReads() method to interface |
core/src/main/java/com/scalar/db/storage/jdbc/RdbEngine*.java |
Implemented isolation level logic for MySQL, PostgreSQL, Oracle, SQL Server, SQLite, and DB2 |
core/src/main/java/com/scalar/db/storage/jdbc/JdbcAdmin.java |
Changed getStorageInfo() to dynamically determine consistent read guarantee based on isolation level |
core/src/main/java/com/scalar/db/storage/cassandra/CassandraAdmin.java |
Updated to pass false for consistent virtual table reads |
core/src/main/java/com/scalar/db/storage/cosmos/CosmosAdmin.java |
Updated to pass false for consistent virtual table reads |
core/src/main/java/com/scalar/db/storage/dynamo/DynamoAdmin.java |
Updated to pass false for consistent virtual table reads |
core/src/main/java/com/scalar/db/storage/objectstorage/ObjectStorageAdmin.java |
Updated to pass false for consistent virtual table reads |
core/src/main/java/com/scalar/db/storage/objectstorage/ObjectStorageWrapper.java |
Enhanced documentation for close() method (unrelated to main feature) |
core/src/main/java/com/scalar/db/storage/multistorage/MultiStorageAdmin.java |
Updated to forward the new field from underlying storage |
| Test files | Updated all test instantiations of StorageInfoImpl to include the new boolean parameter and added comprehensive tests for isolation level behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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 comments and suggestions.
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!
But, I left one suggestion. PTAL!
core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineStrategy.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!
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! 👍
Description
This PR introduces a new method
isConsistentVirtualTableReadGuaranteed()method to theStorageInfointerface to indicate whether a storage guarantees consistent reads for virtual tables.Implementation:
false(doesn't support virtual tables)false(doesn't support virtual tables)false(doesn't support virtual tables)trueif isolation level is REPEATABLE READ or SERIALIZABLEtrueif isolation level is REPEATABLE READ or SERIALIZABLEtrueif isolation level is SERIALIZABLEtrueif isolation level is REPEATABLE READ or SERIALIZABLEtrueif isolation level is READ COMMITTED, REPEATABLE READ, or SERIALIZABLEtrueif isolation level is REPEATABLE READ or SERIALIZABLERelated issues and/or PRs
N/A
Changes made
StorageInfo#isConsistentVirtualTableReadGuaranteed()method to indicate whether a storage guarantees consistent reads for virtual tablesChecklist
Additional notes (optional)
N/A
Release notes
Added
isConsistentVirtualTableReadGuaranteed()method to theStorageInfointerface to indicate whether a storage guarantees consistent reads for virtual tables.