logpuller: extract region request scheduler from subscription client#5665
logpuller: extract region request scheduler from subscription client#5665lidezhu wants to merge 6 commits into
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
do-not-merge/work-in-progress
|
[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 |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
ti-chi-bot
Bot
added
the
size/XL
![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 region request scheduling logic in the logpuller package by introducing a dedicated regionRequestScheduler and requestedStore to manage workers connected to TiKV stores, decoupling this responsibility from subscriptionClient. The review feedback suggests adding defensive nil checks for worker.admission in the submit, close, and inflightCount methods of requestedStore to prevent potential nil pointer dereferences during testing or partial initialization.
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.
| func (s *requestedStore) submit(task *regionPriorityTask) bool { | ||
| if len(s.workers) == 0 { | ||
| return false | ||
| } | ||
| index := (s.nextWorker.Add(1) - 1) % uint64(len(s.workers)) | ||
| return s.workers[index].admission.submit(task) | ||
| } |
There was a problem hiding this comment.
Add a defensive nil check for worker.admission to prevent potential nil pointer dereferences during testing or if workers are partially initialized.
func (s *requestedStore) submit(task *regionPriorityTask) bool {\n\tif len(s.workers) == 0 {\n\t\treturn false\n\t}\n\tindex := (s.nextWorker.Add(1) - 1) % uint64(len(s.workers))\n\tworker := s.workers[index]\n\tif worker.admission == nil {\n\t\treturn false\n\t}\n\treturn worker.admission.submit(task)\n}References
- Enforce defensive programming by ensuring appropriate nil checks exist before object property accesses to prevent nil pointer dereferences.
| func (s *requestedStore) close() { | ||
| for _, worker := range s.workers { | ||
| worker.admission.close() | ||
| } | ||
| } |
There was a problem hiding this comment.
Add a defensive nil check for worker.admission in close() to prevent potential nil pointer dereferences during testing or if workers are partially initialized.
func (s *requestedStore) close() {\n\tfor _, worker := range s.workers {\n\t\tif worker.admission != nil {\n\t\t\tworker.admission.close()\n\t\t}\n\t}\n}References
- Enforce defensive programming by ensuring appropriate nil checks exist before object property accesses to prevent nil pointer dereferences.
| func (s *requestedStore) inflightCount() int { | ||
| count := 0 | ||
| for _, worker := range s.workers { | ||
| count += worker.admission.stats().inflight | ||
| } | ||
| return count | ||
| } |
There was a problem hiding this comment.
Add a defensive nil check for worker.admission in inflightCount() to prevent potential nil pointer dereferences during testing or if workers are partially initialized.
func (s *requestedStore) inflightCount() int {\n\tcount := 0\n\tfor _, worker := range s.workers {\n\t\tif worker.admission != nil {\n\t\t\tcount += worker.admission.stats().inflight\n\t\t}\n\t}\n\treturn count\n}References
- Enforce defensive programming by ensuring appropriate nil checks exist before object property accesses to prevent nil pointer dereferences.
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
ti-chi-bot
Bot
added
size/XXL
1 participant
What problem does this PR solve?
Issue Number: close #xxx
What is changed and how it works?
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