ddl: observe TiKV capacity and ingested SST for add index#69712
ddl: observe TiKV capacity and ingested SST for add index#69712expxiaoli wants to merge 1 commit into
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note-none
|
[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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (11)
📝 WalkthroughWalkthroughThis PR adds PD-based TiKV capacity observation for add-index scheduling, records ingested SST byte metadata across cloud, read-index, and classic ingest paths, and extends SST response metadata with a size field and accessors. ChangesObservability feature
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: context deadline exceeded" 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 (3)
pkg/ingestor/ingestctrl/job_worker_test.go (1)
511-525: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest only covers the
!okbranch of the next-gen recording path.
writeResult: &tikvWriteResult{}leavesnextGenWriteRespnil, soGetSSTMeta()always returnsok=falsehere, and the assertion[]recordedIngestedSST{{}}only exercises the empty-record branch injob_worker.go'singest(). The success branch (recordIngestedSST(w.collector, "next-gen/"+strconv.FormatInt(id, 10), size)) has no test coverage in this file.Consider adding a case where
nextGenWriteResphas valid SST metadata to verify the populated-identity recording path.🤖 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/ingestor/ingestctrl/job_worker_test.go` around lines 511 - 525, The “ingest success” test in job_worker_test.go only covers the empty next-gen record path because `tikvWriteResult` has no `nextGenWriteResp`, so `job_worker.go`’s `ingest()` never exercises the `recordIngestedSST(...)` branch. Add a test case using `regionJob` with a populated `nextGenWriteResp`/SST metadata so `GetSSTMeta()` returns ok and verify the collector records the expected non-empty identity and size via `recordIngestedSST` in addition to the existing nil-metadata case.pkg/ddl/backfilling_import_cloud.go (2)
349-360: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRedundant reliability condition.
observation.count > 0 && observation.bytes == 0is already implied byobservation.zeroSizeCount > 0for the classic source: sincesizeis auint64,bytescan only be zero when every recorded entry had size 0, which meanszeroSizeCount == count. The extra clause is dead weight but harmless.🤖 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_import_cloud.go` around lines 349 - 360, The reliability check in the observation switch has a redundant classic-source condition: in the logic around the observation struct, `observation.count > 0 && observation.bytes == 0` is already covered by `observation.zeroSizeCount > 0`. Update the `switch` branch for `ingestedSSTBytesSourceClassic` to rely only on the zero-size case, keeping the `observation.reliable` and `observation.reason` assignments unchanged.
271-322: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument concurrency guarantees for the new recorder types.
ingestCollectorandingestedSSTRecorderare new mutex-protected shared state accessed concurrently from ingest callbacks and the executor goroutine, but neither has a comment describing the thread-safety contract (e.g., thatRecordIngestedSSTmay be called concurrently whileSnapshot/Resetrun on the executor goroutine).As per coding guidelines, "Comments SHOULD explain non-obvious intent, constraints, invariants, concurrency guarantees, SQL/compatibility contracts, or important performance trade-offs."
🤖 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_import_cloud.go` around lines 271 - 322, Add a concurrency comment for the new recorder types in backfilling import flow: document that ingest callbacks may invoke ingestCollector.Processed and ingestCollector.RecordIngestedSST concurrently, while ingestedSSTRecorder’s state is protected by its mutex. Update the struct/comment near ingestCollector and ingestedSSTRecorder to state the thread-safety contract, including that RecordIngestedSST is safe for concurrent use and any snapshot/reset-style access must remain serialized on the executor goroutine.Source: Coding guidelines
🤖 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/index.go`:
- Around line 3368-3372: The task-end capacity observation in the add-index
reorg flow is skipped whenever g.Wait() returns an error, so terminal outcomes
like KV disk-full auto-pause never capture the end snapshot. Update the
error-handling path around g.Wait() in the add-index reorg code so
w.logTiKVCapacityForAddIndexObservation is recorded for terminal/paused
completions, especially ErrKVDiskFull-related cases, before returning. Use the
existing reorgInfo.mergingTmpIdx check and the observedTiKVUsagePhaseTaskEnd
symbol to keep the snapshot logic consistent.
---
Nitpick comments:
In `@pkg/ddl/backfilling_import_cloud.go`:
- Around line 349-360: The reliability check in the observation switch has a
redundant classic-source condition: in the logic around the observation struct,
`observation.count > 0 && observation.bytes == 0` is already covered by
`observation.zeroSizeCount > 0`. Update the `switch` branch for
`ingestedSSTBytesSourceClassic` to rely only on the zero-size case, keeping the
`observation.reliable` and `observation.reason` assignments unchanged.
- Around line 271-322: Add a concurrency comment for the new recorder types in
backfilling import flow: document that ingest callbacks may invoke
ingestCollector.Processed and ingestCollector.RecordIngestedSST concurrently,
while ingestedSSTRecorder’s state is protected by its mutex. Update the
struct/comment near ingestCollector and ingestedSSTRecorder to state the
thread-safety contract, including that RecordIngestedSST is safe for concurrent
use and any snapshot/reset-style access must remain serialized on the executor
goroutine.
In `@pkg/ingestor/ingestctrl/job_worker_test.go`:
- Around line 511-525: The “ingest success” test in job_worker_test.go only
covers the empty next-gen record path because `tikvWriteResult` has no
`nextGenWriteResp`, so `job_worker.go`’s `ingest()` never exercises the
`recordIngestedSST(...)` branch. Add a test case using `regionJob` with a
populated `nextGenWriteResp`/SST metadata so `GetSSTMeta()` returns ok and
verify the collector records the expected non-empty identity and size via
`recordIngestedSST` in addition to the existing nil-metadata case.
🪄 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: 1d5beac2-84da-4226-a3d6-3fadd94c91e6
📒 Files selected for processing (15)
pkg/ddl/BUILD.bazelpkg/ddl/backfilling_clean_s3.gopkg/ddl/backfilling_dist_executor.gopkg/ddl/backfilling_import_cloud.gopkg/ddl/backfilling_read_index.gopkg/ddl/index.gopkg/ddl/reorg_util_test.gopkg/dxf/framework/taskexecutor/execute/interface.gopkg/ingestor/ingestcli/client.gopkg/ingestor/ingestcli/client_test.gopkg/ingestor/ingestcli/interface.gopkg/ingestor/ingestctrl/BUILD.bazelpkg/ingestor/ingestctrl/job_worker.gopkg/ingestor/ingestctrl/job_worker_test.gopkg/ingestor/ingestctrl/region_job.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69712 +/- ##
================================================
- Coverage 76.3227% 75.8542% -0.4686%
================================================
Files 2041 2079 +38
Lines 560399 580392 +19993
================================================
+ Hits 427712 440252 +12540
- Misses 131786 138030 +6244
- Partials 901 2110 +1209
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
removed
do-not-merge/needs-linked-issue
do-not-merge/needs-triage-completed
labels
expxiaoli
force-pushed
the
tikv_precheck_PR_observe
branch
from
69acd2e to
55fc6a9
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/ddl/index.go (1)
3562-3586: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent log field key naming in new observation logs.
This function mixes
"jobID"/"taskID"(camelCase),"task-key"(kebab-case), and"observation_phase"/"tikv_total_bytes"(snake_case) within the same log calls, while the rest of this file consistently uses kebab-case keys like"job-id"/"task-id"(see lines 3195-3197). Inconsistent keys make these new structured logs harder to grep/aggregate — a real cost for an observability-focused feature.✏️ Align keys with existing file convention
- logutil.DDLLogger().Warn("skip TiKV capacity observation for add-index task because TiKV capacity snapshot failed", - zap.Int64("jobID", jobID), - zap.Int64("taskID", taskID), + logutil.DDLLogger().Warn("skip TiKV capacity observation for add-index task because TiKV capacity snapshot failed", + zap.Int64("job-id", jobID), + zap.Int64("task-id", taskID), zap.String("task-key", taskKey), zap.String("observation_phase", phase), zap.Error(err))(apply similarly to the other two log calls in this function)
As per coding guidelines: "Go code: Follow existing package-local conventions first and keep style consistent with nearby files."
🤖 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/index.go` around lines 3562 - 3586, The new TiKV capacity observation logs in the add-index flow use inconsistent structured field names; align them with the existing kebab-case convention used elsewhere in this file. Update the log calls in the capacity observation helper to use the same key style as nearby DDLLogger usages, especially for job and task identifiers, and apply the same naming consistently across the warn/info branches.Source: Coding guidelines
pkg/ingestor/ingestctrl/job_worker_test.go (1)
511-546: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider consolidating the two near-identical "ingest success" subtests.
Both subtests (Lines 511-525 and 527-545) share the exact same setup/assertion shape and differ only in
writeResult.nextGenWriteRespand the expected recorded snapshot. A table-driven variant would reduce duplication, though the current form is clear and functionally correct.🤖 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/ingestor/ingestctrl/job_worker_test.go` around lines 511 - 546, The two `t.Run` cases in `job_worker_test.go` for `cloudW.ingest` are nearly identical and should be consolidated into a single table-driven test. Keep the shared setup around `recordingIngestedSSTCollector`, `regionJob`, `mockIngestCli.EXPECT().Ingest`, and `ctrl.Satisfied()`, and vary only `writeResult.nextGenWriteResp` plus the expected `collector.snapshot()` for each case.
🤖 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/index.go`:
- Around line 3267-3269: The add-index capacity observation is currently
executed inline in the critical DDL flow, which can block task submission and
task completion; update the call sites around
logTiKVCapacityForAddIndexObservation in the add-index reorg path to run
asynchronously in a background goroutine instead of waiting synchronously. Keep
the existing warning-only failure handling inside
logTiKVCapacityForAddIndexObservation, and apply the same non-blocking change to
all referenced call sites so the DDL path never waits on PD capacity logging.
---
Nitpick comments:
In `@pkg/ddl/index.go`:
- Around line 3562-3586: The new TiKV capacity observation logs in the add-index
flow use inconsistent structured field names; align them with the existing
kebab-case convention used elsewhere in this file. Update the log calls in the
capacity observation helper to use the same key style as nearby DDLLogger
usages, especially for job and task identifiers, and apply the same naming
consistently across the warn/info branches.
In `@pkg/ingestor/ingestctrl/job_worker_test.go`:
- Around line 511-546: The two `t.Run` cases in `job_worker_test.go` for
`cloudW.ingest` are nearly identical and should be consolidated into a single
table-driven test. Keep the shared setup around `recordingIngestedSSTCollector`,
`regionJob`, `mockIngestCli.EXPECT().Ingest`, and `ctrl.Satisfied()`, and vary
only `writeResult.nextGenWriteResp` plus the expected `collector.snapshot()` for
each case.
🪄 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: ccf1b67b-daf0-4855-ba2b-ce512129ac0d
📒 Files selected for processing (13)
pkg/ddl/backfilling_dist_executor.gopkg/ddl/backfilling_import_cloud.gopkg/ddl/backfilling_read_index.gopkg/ddl/index.gopkg/ddl/reorg_util_test.gopkg/dxf/framework/taskexecutor/execute/interface.gopkg/ingestor/ingestcli/client.gopkg/ingestor/ingestcli/client_test.gopkg/ingestor/ingestcli/interface.gopkg/ingestor/ingestctrl/BUILD.bazelpkg/ingestor/ingestctrl/job_worker.gopkg/ingestor/ingestctrl/job_worker_test.gopkg/ingestor/ingestctrl/region_job.go
💤 Files with no reviewable changes (1)
- pkg/dxf/framework/taskexecutor/execute/interface.go
✅ Files skipped from review due to trivial changes (2)
- pkg/ddl/backfilling_dist_executor.go
- pkg/ingestor/ingestcli/client.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/ingestor/ingestcli/interface.go
- pkg/ingestor/ingestctrl/BUILD.bazel
- pkg/ingestor/ingestcli/client_test.go
- pkg/ingestor/ingestctrl/region_job.go
- pkg/ingestor/ingestctrl/job_worker.go
- pkg/ddl/backfilling_read_index.go
expxiaoli
force-pushed
the
tikv_precheck_PR_observe
branch
from
55fc6a9 to
90689d6
Compare
ti-chi-bot
Bot
added
size/XL
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/ddl/index.go (1)
3574-3583: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
errorspackage instead offmt.Errorffor consistency and stack traces.This function otherwise uses
errors.New/errors.Annotatef(lines 3589, 3599, 3603) fromgithub.com/pingcap/errors, which capture stack traces useful for debugging. The type-assertion failure at line 3577 usesfmt.Errorf, losing that context and breaking local consistency.♻️ Proposed fix
- return nil, fmt.Errorf("store %T does not implement helper.Storage", store) + return nil, errors.Errorf("store %T does not implement helper.Storage", store)As per coding guidelines: "Go code: Follow existing package-local conventions first and keep style consistent with nearby files."
🤖 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/index.go` around lines 3574 - 3583, In collectTiKVStoreCapacity, replace the fmt.Errorf used for the helper.Storage type-assertion failure with the github.com/pingcap/errors package to match the surrounding error handling style and preserve stack traces. Keep the existing behavior the same, but return the error using the local errors convention already used in this function (alongside errors.New and errors.Annotatef) so the failure is consistent and debuggable.Source: Coding guidelines
🤖 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/index.go`:
- Around line 3264-3266: The add-index flow only records TiKV capacity before
task submission, so the end-of-task snapshot is missing. Update the add-index
path around g.Wait() in the relevant reorg/add-index logic to call
w.logTiKVCapacityForAddIndexObservation again after the wait completes, ensuring
the final capacity state is captured for both success and failure cases.
---
Nitpick comments:
In `@pkg/ddl/index.go`:
- Around line 3574-3583: In collectTiKVStoreCapacity, replace the fmt.Errorf
used for the helper.Storage type-assertion failure with the
github.com/pingcap/errors package to match the surrounding error handling style
and preserve stack traces. Keep the existing behavior the same, but return the
error using the local errors convention already used in this function (alongside
errors.New and errors.Annotatef) so the failure is consistent and debuggable.
🪄 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: 104ae46b-f137-4f7b-92c7-ed4a972e227b
📒 Files selected for processing (13)
pkg/ddl/backfilling_dist_executor.gopkg/ddl/backfilling_import_cloud.gopkg/ddl/backfilling_read_index.gopkg/ddl/index.gopkg/ddl/reorg_util_test.gopkg/dxf/framework/taskexecutor/execute/interface.gopkg/ingestor/ingestcli/client.gopkg/ingestor/ingestcli/client_test.gopkg/ingestor/ingestcli/interface.gopkg/ingestor/ingestctrl/BUILD.bazelpkg/ingestor/ingestctrl/job_worker.gopkg/ingestor/ingestctrl/job_worker_test.gopkg/ingestor/ingestctrl/region_job.go
💤 Files with no reviewable changes (1)
- pkg/dxf/framework/taskexecutor/execute/interface.go
✅ Files skipped from review due to trivial changes (1)
- pkg/ddl/backfilling_dist_executor.go
🚧 Files skipped from review as they are similar to previous changes (10)
- pkg/ingestor/ingestcli/client_test.go
- pkg/ingestor/ingestctrl/BUILD.bazel
- pkg/ingestor/ingestcli/interface.go
- pkg/ingestor/ingestctrl/job_worker.go
- pkg/ingestor/ingestcli/client.go
- pkg/ingestor/ingestctrl/region_job.go
- pkg/ddl/reorg_util_test.go
- pkg/ingestor/ingestctrl/job_worker_test.go
- pkg/ddl/backfilling_read_index.go
- pkg/ddl/backfilling_import_cloud.go
| if !reorgInfo.mergingTmpIdx { | ||
| w.logTiKVCapacityForAddIndexObservation(ctx, job.ID, taskKey) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore the end-of-task TiKV capacity snapshot. The current path only logs capacity before task submission; add the matching observation after g.Wait() so add-index still captures the end state on both success and failure.
🤖 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/index.go` around lines 3264 - 3266, The add-index flow only records
TiKV capacity before task submission, so the end-of-task snapshot is missing.
Update the add-index path around g.Wait() in the relevant reorg/add-index logic
to call w.logTiKVCapacityForAddIndexObservation again after the wait completes,
ensuring the final capacity state is captured for both success and failure
cases.
90689d6 to
fa03b0f
Compare
|
/retest-required |
1 participant
What problem does this PR solve?
Issue Number: ref #68354
Problem Summary:
This PR is split from #68490.
Before adding TiKV disk-space prediction and enforcement for add index, we need observability data to know:
The observability data for ingested SST bytes should be used only for post-run validation. It should not become part of the add-index execution state, task meta, or subtask summary.
What changed and how does it work?
This PR adds record-only observability for add-index distributed backfill:
SubtaskSummary; there is no task-end aggregation or writeback.Compared with #68490, this PR intentionally makes the following changes:
InitialTiKVCapacitytask meta field and the ingested SST fields fromSubtaskSummary. They are changed to pure log observations.sourcegrouping layer from the diagnostic fields iningestedSSTRecorder. The extrasourcelayer does not add much diagnostic value.This PR does not reject add-index tasks, does not add disk-space prediction, and does not change add-index scheduling behavior.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Bug Fixes