feat(transaction): add Transaction::start_ts and scan_unbounded - #554
feat(transaction): add Transaction::start_ts and scan_unbounded#554ariesdevil wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @ariesdevil! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughTransaction adds lazy, paginated unbounded range scanning through a public ChangesTransaction scanning
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Snapshot
participant Transaction
participant PdClient
Snapshot->>Transaction: scan_unbounded(range)
Transaction->>PdClient: scan(start, end, batch size)
PdClient-->>Transaction: page of KvPair results
Transaction->>PdClient: scan(next key, end, batch size)
PdClient-->>Transaction: final page of KvPair results
Transaction-->>Snapshot: KvPair results
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
da909e8 to
dd640ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/transaction/transaction.rs`:
- Around line 122-127: Update the documentation for the transaction start
timestamp accessor to describe start_ts solely as the transaction’s read
timestamp. Remove claims that writes become visible at this timestamp or that
its physical component is suitable for commit, lease, or fencing semantics;
clarify that visibility is determined by the commit timestamp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e1b0c858-08d6-4555-82b4-3d9b87eec3e9
📒 Files selected for processing (2)
src/transaction/snapshot.rssrc/transaction/transaction.rs
2bfa214 to
297da6c
Compare
| if let Some(last) = page.last() { | ||
| start = last.key().clone().next_key(); | ||
| } | ||
| out.extend(page); |
There was a problem hiding this comment.
We can't estimate the data size in a range, so buffer them in memory is danger to me, it can easily cause OOM in the client side.
There was a problem hiding this comment.
Fixed, aligned to the client-go's behavior, PTAL again.
297da6c to
f2bd080
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/transaction/transaction.rs`:
- Around line 122-134: Consolidate the duplicate accessors by making either
start_ts or the existing start_timestamp delegate to the other, with both
preserving the current timestamp value and public API behavior. Prefer retaining
the established accessor unless the project’s compatibility policy explicitly
phases it out, and avoid maintaining two independent implementations.
- Around line 1246-1248: Reconcile SCAN_UNBOUNDED_BATCH_SIZE with the documented
scan batch size: choose the intended value (256 or 1024), update the constant or
its description accordingly, and replace the inaccurate client-go
DefaultScanBatchSize reference with a valid upstream source or remove the
attribution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 20b28e3b-75ad-4ce5-894d-12ed305a281b
📒 Files selected for processing (4)
src/lib.rssrc/transaction/mod.rssrc/transaction/snapshot.rssrc/transaction/transaction.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/transaction/snapshot.rs
Signed-off-by: Yijun Zhao <ariesdevil77@gmail.com>
f2bd080 to
69be17a
Compare
start_ts exposes the transaction's TSO start timestamp, matching client-go's KVTxn.StartTS.
scan_unbounded (on Transaction and Snapshot) returns all pairs in a range by paginating internally in batches of 1024, matching client-go's KVSnapshot.
Iter semantics: a short page means the range is exhausted, a full page advances past the last key.
Summary by CodeRabbit