pkg/ddl, pkg/dxf: add local sort disk check and logging for backfill workers#68320
pkg/ddl, pkg/dxf: add local sort disk check and logging for backfill workers#68320expxiaoli wants to merge 5 commits into
Conversation
ti-chi-bot
Bot
added
the
release-note-none
|
@expxiaoli I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
the
size/L
|
Hi @expxiaoli. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
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:
📝 WalkthroughWalkthroughAdds local-sort disk-usage sampling and structured logging during local ingest, worker-side free-disk validation using tracked job requirements, and task-executor slot snapshot APIs with tests. ChangesLocal Sort Disk Monitoring
Worker-side Disk Precheck
Task-Executor Slot Snapshot
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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 (2)
pkg/ddl/backfilling_read_index.go (2)
120-141: 💤 Low valueConsider documenting the single-use lifecycle constraint.
The
start()method has no guard against being called multiple times, which would leak goroutines. While the current usage inrunLocalPipelineis safe (called exactly once), documenting this constraint would prevent future misuse.📝 Suggested approach
Add a doc comment:
+// start begins sampling. Must be called exactly once before stopAndSnapshot(). func (s *localSortDiskUsageSampler) start() {Or add a guard:
+ started sync.Once func (s *localSortDiskUsageSampler) start() { + s.started.Do(func() { s.mu.Lock() s.startTime = time.Now() // ... rest of method + }) }🤖 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 `@pkg/ddl/backfilling_read_index.go` around lines 120 - 141, The start() method on localSortDiskUsageSampler can be called multiple times and will spawn leaking goroutines; add a single-use guard or document the lifecycle: modify localSortDiskUsageSampler.start to check a boolean (e.g., started) protected by mu (or return early if startTime is non-zero), set started/startTime on the first call, and avoid re-entering so ticker/goroutine are only created once (ensure doneCh/stopCh handling remains correct), or alternatively add a doc comment on localSortDiskUsageSampler.start stating it must only be invoked once.
56-56: 💤 Low valueConsider documenting the sampling interval choice.
A brief comment explaining why 1 second was chosen as the sampling interval would help future maintainers understand the trade-off between sampling accuracy and overhead.
📝 Suggested documentation
+// localSortDiskUsageSampleInterval defines how frequently the sampler +// checks disk usage during local sort operations. 1 second provides +// reasonable granularity without measurable overhead. const localSortDiskUsageSampleInterval = time.Second🤖 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 `@pkg/ddl/backfilling_read_index.go` at line 56, Add a short comment above the constant localSortDiskUsageSampleInterval explaining why a 1 second sampling interval was chosen (e.g., balances timely disk-usage responsiveness during sort/backfill monitoring with low CPU/io overhead), mention any trade-offs (latency vs overhead) and note whether the value is adjustable or tied to other constants (so maintainers know they can tweak it). This comment should live next to the declaration of localSortDiskUsageSampleInterval in backfilling_read_index.go and reference the intent to sample disk usage for local sort/backfill operations.
🤖 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 `@pkg/ddl/backfilling_read_index.go`:
- Around line 87-109: Add clear concurrency and lifecycle documentation for the
sampler types: describe that localSortDiskUsageSampler is safe for concurrent
use only via its Start/Stop (or sampling goroutine) lifecycle, explain ownership
of stopCh/doneCh, that getUsage is called from a single background goroutine
started by the sampler, and that mu protects
startTime/endTime/sumBytes/sampleCnt/peakBytes/lastBytes which are read by
Snapshot via localSortDiskUsageSnapshot; annotate which fields are only touched
by the background goroutine vs protected by mu and state transitions (e.g.,
Start initializes and launches goroutine, Stop closes stopCh and waits on
doneCh), and document expected calling patterns and thread-safety guarantees for
Snapshot, Start, and Stop using the type names localSortDiskUsageSampler and
localSortDiskUsageSnapshot to make locating the docs easy.
---
Nitpick comments:
In `@pkg/ddl/backfilling_read_index.go`:
- Around line 120-141: The start() method on localSortDiskUsageSampler can be
called multiple times and will spawn leaking goroutines; add a single-use guard
or document the lifecycle: modify localSortDiskUsageSampler.start to check a
boolean (e.g., started) protected by mu (or return early if startTime is
non-zero), set started/startTime on the first call, and avoid re-entering so
ticker/goroutine are only created once (ensure doneCh/stopCh handling remains
correct), or alternatively add a doc comment on localSortDiskUsageSampler.start
stating it must only be invoked once.
- Line 56: Add a short comment above the constant
localSortDiskUsageSampleInterval explaining why a 1 second sampling interval was
chosen (e.g., balances timely disk-usage responsiveness during sort/backfill
monitoring with low CPU/io overhead), mention any trade-offs (latency vs
overhead) and note whether the value is adjustable or tied to other constants
(so maintainers know they can tweak it). This comment should live next to the
declaration of localSortDiskUsageSampleInterval in backfilling_read_index.go and
reference the intent to sample disk usage for local sort/backfill operations.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a8111b14-8d26-4e86-9f6e-fe12483b7e9c
📒 Files selected for processing (1)
pkg/ddl/backfilling_read_index.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68320 +/- ##
================================================
- Coverage 76.3193% 75.5169% -0.8024%
================================================
Files 2041 2086 +45
Lines 559929 582918 +22989
================================================
+ Hits 427334 440202 +12868
- Misses 131694 140581 +8887
- Partials 901 2135 +1234
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
added
size/XL
expxiaoli
changed the title
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 `@pkg/ddl/ingest/disk_root.go`:
- Around line 256-266: The current logic subtracts aggregate used bytes from
aggregate required bytes (allJobsRequiredBytes - allJobsUsedBytes) which lets
one job's extra usage mask another job's shortfall; instead compute per-job
shortfalls and sum them: include the new job by computing newJobReq :=
uint64(newJobRequiredSlots) * LocalSortBytesPerSlot and for each runningJob
compute jobReq := uint64(runningJob.RequiredSlots) * LocalSortBytesPerSlot then
if jobReq > runningJob.UsedBytes add (jobReq - runningJob.UsedBytes) to
allJobsGapBytes (and similarly compare newJobReq to 0/its used if applicable),
remove the aggregate subtraction (allJobsRequiredBytes - allJobsUsedBytes), and
then set totalRequiredBytes := totalCapacityBytes/10 + allJobsGapBytes.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1178c9d3-c3f4-4495-84cc-f50eae30947c
📒 Files selected for processing (6)
pkg/ddl/backfilling_dist_executor.gopkg/ddl/ingest/disk_root.gopkg/ddl/ingest/mem_root_test.gopkg/dxf/framework/taskexecutor/slot.gopkg/dxf/framework/taskexecutor/slot_test.gopkg/dxf/framework/taskexecutor/task_executor.go
|
[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 |
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. |
|
@expxiaoli: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
What problem does this PR solve?
Issue Number: close #68293
Problem Summary:
When add index runs with local sort on TiDB workers, we do not have a worker-side admission check for local disk headroom. A new backfill task may start even when the remaining disk is not enough for the ongoing local-sort workload plus the extra 10% headroom reserved for partial import.
Besides, we do not have a summary view of how much local disk space the backfill actually used. This makes it harder to diagnose disk pressure and compare the observed usage with the expected local-sort disk requirement.
What changed and how does it work?
This PR improves both prevention and observability for local-sort add index backfill.
The change:
required_bytes = required_slots * 512MBfor each joball_jobs_required_bytes = sum(required_bytes)for all running jobs plus the new joball_jobs_used_bytes = sum(used_bytes)for running jobsall_jobs_gap_bytes = max(all_jobs_required_bytes - all_jobs_used_bytes, 0)total_required_bytes = total_capacity * 10% + all_jobs_gap_bytestotal_required_bytes, so add index local sort can fail fast before entering a higher-risk execution pathThis keeps the local-sort algorithm unchanged, but makes the worker more defensive under disk pressure and provides better disk-usage visibility for diagnosis.
Check List
Tests
Unit test:
env GOPATH=/Users/xiaoli/go GOSUMDB=sum.golang.org go test -run '^TestRiskOfDiskFull$' -count=1 -tags=intest,deadlock ./pkg/ddl/ingestenv GOPATH=/Users/xiaoli/go GOSUMDB=sum.golang.org go test -run '^TestSlotManager$' -count=1 -tags=intest,deadlock ./pkg/dxf/framework/taskexecutorenv GOPATH=/Users/xiaoli/go GOSUMDB=sum.golang.org go test -run '^(TestBackfillingSchedulerLocalMode|TestBackfillTaskMetaVersion)$' -count=1 -tags=intest,deadlock ./pkg/ddlSide effects
Documentation
Release note
Summary by CodeRabbit