-
Couldn't load subscription status.
- Fork 40
Implement scanner API for single CRUD transactions #2701
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
Implement scanner API for single CRUD transactions #2701
Conversation
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 a full implementation of the getScanner API for single CRUD transactions, augments unit tests to cover its behavior, and removes a placeholder integration test.
- Implement
getScannerinSingleCrudOperationTransactionManagerwith proper exception wrapping. - Add comprehensive unit tests for
one(),all(),iterator(), and error scenarios on the scanner. - Remove the disabled placeholder integration test without replacing it.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| integration-test/src/main/java/.../SingleCrudOperationTransactionIntegrationTestBase.java | Removed a disabled stub test for getScanner without adding a real one. |
| core/src/test/java/.../SingleCrudOperationTransactionManagerTest.java | Added unit tests covering scanner methods and error propagation. |
| core/src/main/java/com/scalar/db/transaction/singlecrudoperation/SingleCrudOperationTransactionManager.java | Implemented getScanner and wrapped storage scanner calls into CrudException. |
Comments suppressed due to low confidence (1)
integration-test/src/main/java/com/scalar/db/transaction/singlecrudoperation/SingleCrudOperationTransactionIntegrationTestBase.java:412
- The disabled placeholder test for
getScannerwas removed without adding an actual integration test to verify scanner behavior against a real storage backend. Consider adding an end-to-end integration test that exercisesgetScanneron committed records.
@Disabled("Implement later")
|
|
||
| return new AbstractTransactionManagerCrudOperableScanner() { | ||
| @Override | ||
| public Optional<Result> one() throws CrudException { |
Copilot
AI
May 28, 2025
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.
[nitpick] The try–catch logic for wrapping ExecutionException into CrudException is duplicated in one() and all(). Consider extracting a helper method (e.g. wrapCrud(Supplier<T>)) to reduce code duplication and improve readability.
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!
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!
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! 👍
8ea08fa
into
add-scanner-api-to-transaction-abstraction
Description
This PR adds a scanner API implementation for single CRUD transactions.
Note that we are working on this feature in the
add-scanner-api-to-transaction-abstractionfeature branch.Related issues and/or PRs
Changes made
Checklist
Additional notes (optional)
N/A
Release notes
N/A