logpuller: initialize spans only after full range coverage#5657
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
do-not-merge/work-in-progress
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe logpuller now tracks initialized key-range coverage across regions, marks spans initialized only after complete coverage, adjusts resolved-timestamp handling, and updates tests to use realistic locked range state. ChangesSpan initialization coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
ti-chi-bot
Bot
added
the
size/M
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the initialization logic of a subscribedSpan by moving the state transition of initialized from handleResolvedTs to tryAdvanceResolvedTs in region_event_handler.go. This ensures that the span is marked as initialized only after its resolved timestamp has been successfully advanced. Additionally, a new unit test TestSpanInitializedAfterResolvedTsAdvanced has been added to verify this behavior, and a descriptive comment was added to the initialized field in span_registry.go. There are no review comments to address, and the changes look correct and well-tested.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
ti-chi-bot
Bot
added
size/L
ti-chi-bot
Bot
added
size/XL
lidezhu
changed the title
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
|
/gemini summary |
Summary of ChangesThis pull request refines the initialization process for logpuller spans. Previously, a span could be marked as initialized prematurely. The changes implement a tracking mechanism that aggregates initialized ranges across regions, ensuring the entire span is fully covered before transitioning to an initialized state. This improves the reliability of the subscription client's initialization phase. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Activity
|
|
/gemini review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a spanInitializationTracker to track the initialization of sub-ranges within a subscribed span. Instead of marking a span as initialized immediately, it now tracks individual initialized ranges and only marks the entire span as initialized once all of its sub-ranges have completed their initial scans. The PR also updates related event handling logic, warning logs, and unit tests to support this new tracking mechanism. I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
logservice/logpuller/span_initialization_tracker_test.go (1)
40-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding test cases for open-ended intervals (infinity bounds).
In TiKV and TiCDC, empty byte slices (
[]byte{}) are often used inStartKeyandEndKeyto represent negative and positive infinity. Consider adding a test case that verifies the tracker correctly handles merging and completion when given empty bounds to ensure open-ended ranges are flawlessly supported.🤖 Prompt for 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. In `@logservice/logpuller/span_initialization_tracker_test.go` around lines 40 - 52, The TestSpanInitializationTrackerMergesOverlappingRanges test only covers finite bounds; add cases using empty StartKey and EndKey values to represent negative and positive infinity. Verify spanInitializationTracker.add merges these open-ended ranges correctly and returns completion only after the total open-ended range is covered, while preserving the existing finite-range assertions.
🤖 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 `@logservice/logpuller/region_event_handler.go`:
- Around line 396-412: In the resolved-timestamp handling flow, clamp ts to
span.startTs before the outer conditional whenever ts exceeds span.startTs while
span.initialized.Load() is false. Keep the existing warning in place, then
evaluate the conditional and subsequent store/return logic using the clamped
timestamp so downstream consumers cannot observe premature progress.
---
Nitpick comments:
In `@logservice/logpuller/span_initialization_tracker_test.go`:
- Around line 40-52: The TestSpanInitializationTrackerMergesOverlappingRanges
test only covers finite bounds; add cases using empty StartKey and EndKey values
to represent negative and positive infinity. Verify
spanInitializationTracker.add merges these open-ended ranges correctly and
returns completion only after the total open-ended range is covered, while
preserving the existing finite-range assertions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4317788-e2f7-44b8-9bcf-8e0ac3cfdc53
📒 Files selected for processing (6)
logservice/logpuller/region_event_handler.gologservice/logpuller/region_event_handler_test.gologservice/logpuller/span_initialization_tracker.gologservice/logpuller/span_initialization_tracker_test.gologservice/logpuller/span_registry.gologservice/logpuller/subscription_client_test.go
|
/test all |
ti-chi-bot
Bot
added
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: asddongmen, hongyunyan The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
3 participants
What problem does this PR solve?
Issue Number: close #5658
What is changed and how it works?
This pull request refines the initialization process for logpuller spans. Previously, a span could be marked as initialized prematurely. The changes implement a tracking mechanism that aggregates initialized ranges across regions, ensuring the entire span is fully covered before transitioning to an initialized state. This improves the reliability of the subscription client's initialization phase.
Highlights
spanInitializationTrackerto ensure that a subscription span is only marked as initialized after all underlying regions within the span have completed their initial scan.region_event_handler.goto delegate initialization tracking to the new tracker instead of immediately marking the span as initialized upon receiving a single event.span_initialization_tracker_test.goand updated existing tests to verify the new initialization logic.Check List
Tests
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note
Summary by CodeRabbit
Bug Fixes
Tests