-
Notifications
You must be signed in to change notification settings - Fork 40
Unify Get/Scan preparation logic for transaction reads and Serializable validation #3113
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
Unify Get/Scan preparation logic for transaction reads and Serializable validation #3113
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 significant refactoring to centralize and unify the preparation logic for 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 is a nice refactoring that unifies the logic for preparing Get and Scan operations. However, this unification has introduced a performance regression in the validation logic for serializable transactions. The validation scans and gets now fetch all columns from storage, whereas previously they only fetched the columns necessary for validation. I've added a couple of review comments with suggestions to address this inefficiency while retaining the benefits of the refactoring.
core/src/main/java/com/scalar/db/transaction/consensuscommit/Snapshot.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/transaction/consensuscommit/Snapshot.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 refactors how Get and Scan operations are prepared for storage in consensus commit transactions. The main changes include:
- Extracting conjunction conversion logic from
CrudHandlertoConsensusCommitUtilsfor reusability - Moving projection clearing and consistency setting to new utility methods
prepareGetForStorageandprepareScanForStorage - Updating test expectations to reflect that operations passed to storage now use the original Get/Scan for snapshot tracking instead of the storage-prepared version
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ConsensusCommitSpecificIntegrationTestBase.java | Fixed bug where manager.get() was incorrectly called instead of transaction.get() |
| SnapshotTest.java | Removed redundant consistency settings and updated test expectations to use original operations for tracking |
| CrudHandlerTest.java | Updated test expectations to pass original operations to snapshot methods and added conjunction handling tests |
| ConsensusCommitUtilsTest.java | Added comprehensive tests for new prepareGetForStorage and prepareScanForStorage utility methods |
| Snapshot.java | Refactored validation logic to use new utility methods and added conjunction filtering |
| CrudHandler.java | Refactored to use new utility methods and pass original operations to snapshot |
| ConsensusCommitUtils.java | Added new utility methods for preparing Get/Scan operations for storage with conjunction conversion |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
core/src/main/java/com/scalar/db/transaction/consensuscommit/Snapshot.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/transaction/consensuscommit/CrudHandler.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/transaction/consensuscommit/Snapshot.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/transaction/consensuscommit/Snapshot.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/transaction/consensuscommit/Snapshot.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/transaction/consensuscommit/Snapshot.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitUtils.java
Show resolved
Hide resolved
core/src/main/java/com/scalar/db/transaction/consensuscommit/ConsensusCommitUtils.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! 👍
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!
…action-reads-and-serializable-validation
…action-reads-and-serializable-validation
Description
This PR unifies the Get/Scan preparation logic used during transaction reads and Serializable validation by introducing common utility methods in
ConsensusCommitUtils. Previously, these two processes prepared Get/Scan objects differently, which could result in inconsistent ordering and cause Serializable validation failures.Related issues and/or PRs
N/A
Changes made
ConsensusCommitUtils.prepareGetForStorage()ConsensusCommitUtils.prepareScanForStorage()CrudHanderandSnapshotto useConsensusCommitUtils.prepareGetForStorage()andConsensusCommitUtils.prepareScanForStorage()to use the same Scan and Get between transaction reads and Serializable validation.Checklist
Additional notes (optional)
N/A
Release notes
Fixed an inconsistency where Get and Scan operations were prepared differently for transaction reads and Serializable validation, which could result in inconsistent ordering and cause Serializable validation failures.