Repository navigation
pkg/ddl, pkg/tici: add TiCI pre-split for DDL global sort ingest | tidb-test=13ccf8de48e8db2290ff884598444d0508606bbf tiflash=feature-fts - #67124
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
do-not-merge/invalid-title
do-not-merge/work-in-progress
|
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 TiCI pre-split orchestration to distributed/global-sort DDL backfilling for full-text/hybrid indexes, extends range-splitting to return group size/key count, introduces a TiCI PreSplitImportShards RPC and client support, and adds tests and failpoints to validate pre-split behavior. Changes
Sequence DiagramsequenceDiagram
participant Scheduler as DDL Scheduler
participant Splitter as RangeSplitter
participant TiCI as TiCI Service
participant Store as KV Storage
Scheduler->>Splitter: SplitOneRangesGroup()
Splitter-->>Scheduler: (endKey, dataFiles, statFiles, groupSize, groupKeyCnt, ...)
Scheduler->>Scheduler: Aggregate groups -> Build PreSplitImportShardsRequest
Scheduler->>TiCI: PreSplitImportShards(request) (1 min timeout)
TiCI-->>Scheduler: PreSplitImportShardsResponse (success / error)
alt Success
Scheduler->>Scheduler: Incorporate TiCI split info into plan
else Failure
Scheduler->>Scheduler: Log error, continue with default plan
end
Scheduler->>Store: Execute global-sort ingest plan
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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/XXL
|
Hi @3pointer. 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. |
3pointer
changed the title
|
/build |
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/ddl/backfilling_dist_scheduler.go (1)
178-186: Avoid an uncheckedstorageWithPDAndCodeccast here.
getUserStoreAndTable()still returnskv.Storage, so this assertion turns any wrapper/mock withoutGetCodec()into a scheduler panic instead of a normal DDL error. Returning an error keeps retries and fallback logic in-band.Possible fix
- return generateGlobalSortIngestPlan( - ctx, - store.(storageWithPDAndCodec), - taskHandle, - task, - &backfillMeta, - backfillMeta.CloudStorageURI, - backfillMeta.ScanSnapshotTS, - logger) + codecStore, ok := store.(storageWithPDAndCodec) + if !ok { + return nil, errors.Errorf("storage %T does not expose codec", store) + } + return generateGlobalSortIngestPlan( + ctx, + codecStore, + taskHandle, + task, + &backfillMeta, + backfillMeta.CloudStorageURI, + backfillMeta.ScanSnapshotTS, + logger)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ddl/backfilling_dist_scheduler.go` around lines 178 - 186, The code currently does an unchecked type assertion to storageWithPDAndCodec when calling generateGlobalSortIngestPlan, which can panic if getUserStoreAndTable returned a kv.Storage wrapper/mock without GetCodec; change this to a safe type check: test whether store is a storageWithPDAndCodec (using a typed assertion with ok) and if not return a proper error from the calling function so the scheduler handles it as a DDL error and keeps retry/fallback logic in-band; update the call site that currently passes store.(storageWithPDAndCodec) into generateGlobalSortIngestPlan to use the validated typed value or return the new error, and ensure error messages reference getUserStoreAndTable/storageWithPDAndCodec to aid debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/ddl/backfilling_dist_scheduler.go`:
- Around line 530-536: The failpoint Inject named
"mockGenerateGlobalSortIngestPlanAfterPreSplit" currently returns before the
pre-split logic so triggerTiCIPreSplitImportShards() never runs; either relocate
the failpoint.Inject block so it is executed after the pre-split section (so
triggerTiCIPreSplitImportShards() runs during tests) or rename the failpoint to
accurately reflect that it short-circuits before pre-split; update the instance
in backfilling_dist_scheduler.go (the failpoint.Inject call and the mock name)
and any tests that enable this failpoint to match the chosen fix.
In `@pkg/ddl/index.go`:
- Around line 1611-1618: The current cloudStorageURI validation runs too early
and can break retries/owner handoffs; move the check into the FULLTEXT job
initialization branch by calling loadCloudStorageURI(job) during model.StateNone
handling and then validating job.ReorgMeta.UseCloudStorage instead of directly
using handle.GetCloudStorageURI(w.workCtx, w.store); specifically, update the
initialization path that handles model.StateNone to call
loadCloudStorageURI(job) and return dbterror.ErrUnsupportedAddColumnarIndex if
job.ReorgMeta.UseCloudStorage is false so subsequent states rely on the fixed
value in job.ReorgMeta rather than a global GetCloudStorageURI call.
---
Nitpick comments:
In `@pkg/ddl/backfilling_dist_scheduler.go`:
- Around line 178-186: The code currently does an unchecked type assertion to
storageWithPDAndCodec when calling generateGlobalSortIngestPlan, which can panic
if getUserStoreAndTable returned a kv.Storage wrapper/mock without GetCodec;
change this to a safe type check: test whether store is a storageWithPDAndCodec
(using a typed assertion with ok) and if not return a proper error from the
calling function so the scheduler handles it as a DDL error and keeps
retry/fallback logic in-band; update the call site that currently passes
store.(storageWithPDAndCodec) into generateGlobalSortIngestPlan to use the
validated typed value or return the new error, and ensure error messages
reference getUserStoreAndTable/storageWithPDAndCodec to aid debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 511225db-7053-4211-b42b-f69d16c4588a
⛔ Files ignored due to path filters (1)
pkg/tici/tici.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (11)
pkg/ddl/backfilling_dist_scheduler.gopkg/ddl/backfilling_dist_scheduler_test.gopkg/ddl/index.gopkg/dxf/importinto/planner.gopkg/lightning/backend/external/merge_v2.gopkg/lightning/backend/external/split.gopkg/lightning/backend/external/split_test.gopkg/lightning/backend/external/testutil.gopkg/tici/tici.protopkg/tici/tici_manager_client.gopkg/tici/tici_manager_client_test.go
| cloudStorageURI := handle.GetCloudStorageURI(w.workCtx, w.store) | ||
| // DXF pre-split index requires cloud storage. | ||
| // local ingest doesn't keep a global order and pre-split is not supported. | ||
| // FullText index already communiate with tici a cloud storage uri. | ||
| // TODO: Another fallback is to use the cloud storage uri from tici if not set. | ||
| if len(cloudStorageURI) == 0 { | ||
| return ver, errors.Trace(dbterror.ErrUnsupportedAddColumnarIndex.FastGen("FULLTEXT index requires cloud storage")) | ||
| } |
There was a problem hiding this comment.
Validate cloud storage only when the FULLTEXT job is initialized.
Because this runs before the indexInfo.State switch, a FULLTEXT job that already passed StateNone can start failing on retry / owner handoff if the global cloud-storage URI is changed or cleared mid-DDL. The chosen URI should be fixed once during setup and reused from job/task state afterward.
Possible fix
- cloudStorageURI := handle.GetCloudStorageURI(w.workCtx, w.store)
- // DXF pre-split index requires cloud storage.
- // local ingest doesn't keep a global order and pre-split is not supported.
- // FullText index already communiate with tici a cloud storage uri.
- // TODO: Another fallback is to use the cloud storage uri from tici if not set.
- if len(cloudStorageURI) == 0 {
- return ver, errors.Trace(dbterror.ErrUnsupportedAddColumnarIndex.FastGen("FULLTEXT index requires cloud storage"))
- }case model.StateNone:
loadCloudStorageURI(w, job)
if !job.ReorgMeta.UseCloudStorage {
return ver, errors.Trace(dbterror.ErrUnsupportedAddColumnarIndex.FastGen("FULLTEXT index requires cloud storage"))
}
// continue initialization...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/ddl/index.go` around lines 1611 - 1618, The current cloudStorageURI
validation runs too early and can break retries/owner handoffs; move the check
into the FULLTEXT job initialization branch by calling loadCloudStorageURI(job)
during model.StateNone handling and then validating
job.ReorgMeta.UseCloudStorage instead of directly using
handle.GetCloudStorageURI(w.workCtx, w.store); specifically, update the
initialization path that handles model.StateNone to call
loadCloudStorageURI(job) and return dbterror.ErrUnsupportedAddColumnarIndex if
job.ReorgMeta.UseCloudStorage is false so subsequent states rely on the fixed
value in job.ReorgMeta rather than a global GetCloudStorageURI call.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/fts #67124 +/- ##
===================================================
- Coverage 76.8610% 76.7511% -0.1099%
===================================================
Files 1960 1962 +2
Lines 555677 558695 +3018
===================================================
+ Hits 427099 428805 +1706
- Misses 127116 128424 +1308
- Partials 1462 1466 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
3pointer
changed the title
|
/test mysql-test |
|
@3pointer: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/ddl/backfilling_dist_scheduler.go (2)
816-850: Consider using actualgroupSizeinstead of estimated value.Line 816 captures
groupSizeandgroupKeyCntfromSplitOneRangesGroup()but ignores them. However, line 849 uses an estimated valuekvMeta.TotalKVSize / uint64(instanceCnt)forTotalKVSizein the metadata.The actual
groupSizereturned by the splitter would be more accurate. The TiCI pre-split code at line 738 correctly uses the actualgroupSize.♻️ Suggested improvement
- endKeyOfGroup, dataFiles, statFiles, _, _, interiorRangeJobKeys, interiorRegionSplitKeys, err := splitter.SplitOneRangesGroup() + endKeyOfGroup, dataFiles, statFiles, groupSize, _, interiorRangeJobKeys, interiorRegionSplitKeys, err := splitter.SplitOneRangesGroup() // ... existing code ... m := &BackfillSubTaskMeta{ MetaGroups: []*external.SortedKVMeta{{ StartKey: startKey, EndKey: endKey, - TotalKVSize: kvMeta.TotalKVSize / uint64(instanceCnt), + TotalKVSize: groupSize, }},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ddl/backfilling_dist_scheduler.go` around lines 816 - 850, The code calls splitter.SplitOneRangesGroup() which returns groupSize but the value is ignored and TotalKVSize in BackfillSubTaskMeta.MetaGroups is set to the coarse kvMeta.TotalKVSize / uint64(instanceCnt); change this to use the actual groupSize returned by SplitOneRangesGroup() (the identifier groupSize) when constructing the MetaGroups entry in the BackfillSubTaskMeta to provide an accurate TotalKVSize, i.e., replace the computed expression with groupSize (casting if necessary to uint64) where TotalKVSize is assigned.
656-680: Failpoint and production code handle nilkvMetadifferently.The failpoint at lines 658-660 silently skips nil entries with
continue, while the production code at line 680 returns an error. This inconsistency could mask bugs during testing where nil entries would silently succeed in tests but fail in production.Consider aligning the behaviors or documenting this intentional difference.
♻️ Suggested fix for consistency
failpoint.Inject("mockBuildTiCIPreSplitReportGroups", func() { reportGroups := make([]*tici.PreSplitImportShardMeta, 0, len(kvMetaGroups)) for i, kvMeta := range kvMetaGroups { if kvMeta == nil { - continue + failpoint.Return(nil, errors.Errorf("subtask kv group %d is empty", i)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ddl/backfilling_dist_scheduler.go` around lines 656 - 680, The failpoint Inject block (failpoint.Inject / mockBuildTiCIPreSplitReportGroups) currently skips nil entries in kvMetaGroups while the production loop over kvMetaGroups (where PreSplitImportShardMeta objects are built) returns an error on nil kvMeta; make them consistent by changing the failpoint behavior to mirror production: when iterating kvMetaGroups in the failpoint, detect nil kvMeta and return an error (similar text to "subtask kv group %d is empty") instead of silently continuing, so tests fail the same way as production; alternatively, if you intended to allow nil entries, change the production loop to skip like the failpoint—pick one approach and apply it to both the failpoint.Inject block and the production loop that builds reportGroups (referencing kvMetaGroups, eleIDs, and PreSplitImportShardMeta).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/ddl/backfilling_dist_scheduler.go`:
- Around line 178-186: The call to generateGlobalSortIngestPlan uses an
unchecked type assertion store.(storageWithPDAndCodec) which can panic; replace
it with a checked assertion (e.g., storeWithCodec, ok :=
store.(storageWithPDAndCodec)) and if !ok return an error (or wrap an existing
error type) indicating "store does not implement storageWithPDAndCodec" before
calling generateGlobalSortIngestPlan, then pass storeWithCodec into
generateGlobalSortIngestPlan instead of the direct assertion.
---
Nitpick comments:
In `@pkg/ddl/backfilling_dist_scheduler.go`:
- Around line 816-850: The code calls splitter.SplitOneRangesGroup() which
returns groupSize but the value is ignored and TotalKVSize in
BackfillSubTaskMeta.MetaGroups is set to the coarse kvMeta.TotalKVSize /
uint64(instanceCnt); change this to use the actual groupSize returned by
SplitOneRangesGroup() (the identifier groupSize) when constructing the
MetaGroups entry in the BackfillSubTaskMeta to provide an accurate TotalKVSize,
i.e., replace the computed expression with groupSize (casting if necessary to
uint64) where TotalKVSize is assigned.
- Around line 656-680: The failpoint Inject block (failpoint.Inject /
mockBuildTiCIPreSplitReportGroups) currently skips nil entries in kvMetaGroups
while the production loop over kvMetaGroups (where PreSplitImportShardMeta
objects are built) returns an error on nil kvMeta; make them consistent by
changing the failpoint behavior to mirror production: when iterating
kvMetaGroups in the failpoint, detect nil kvMeta and return an error (similar
text to "subtask kv group %d is empty") instead of silently continuing, so tests
fail the same way as production; alternatively, if you intended to allow nil
entries, change the production loop to skip like the failpoint—pick one approach
and apply it to both the failpoint.Inject block and the production loop that
builds reportGroups (referencing kvMetaGroups, eleIDs, and
PreSplitImportShardMeta).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4ac67be6-60bc-4336-be28-b475299249e3
⛔ Files ignored due to path filters (1)
pkg/tici/tici.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (1)
pkg/ddl/backfilling_dist_scheduler.go
OliverS929
left a comment
OliverS929
left a comment
There was a problem hiding this comment.
It looks like this PR may not cover all tici-index paths for global sort ingest yet. At the moment, only the DDL-related path seems to be implemented, while import-into does not appear to be included. If that is the intended scope, could we make it explicit in the PR description or comments?
| rpc GetImportStoragePrefix(GetImportStoragePrefixRequest) | ||
| returns (GetImportStoragePrefixResponse); | ||
|
|
||
| // Called before Import Into full index build to pre-split shards by |
There was a problem hiding this comment.
I don't think this PR has included the pre-split support for Import Into related path. If that is beyond the scope of this PR, please change the comment accordingly and add an TODO item for Import Into support.
| require.Equal(t, proto.StepDone, task.Step) | ||
| } | ||
|
|
||
| func TestBackfillingSchedulerGlobalSortModeTiCIPreSplit(t *testing.T) { |
There was a problem hiding this comment.
If I understand this unit test correctly, this unit test has mockGenerateGlobalSortIngestPlanAfterPreSplit enabled, which actually skipped through the actual pre-split implementation, and thus does not provide coverage for the function implementation itself.
| var lastEndKey []byte | ||
| notExhausted: | ||
| endKey, dataFiles, statFiles, rangeJobKeys, regionSplitKeys, err := splitter.SplitOneRangesGroup() | ||
| endKey, dataFiles, statFiles, _, _, rangeJobKeys, regionSplitKeys, err := splitter.SplitOneRangesGroup() |
There was a problem hiding this comment.
I don't think directly shorting the return value here in this test file will provide coverage for the newly-introduced behavior of RangeSplitter.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/ddl/backfilling_dist_scheduler_internal_test.go (1)
1-13: Consider updating license year for new file.This is a new file created in 2026, but uses "Copyright 2023" in the license header. Per coding guidelines, new source files should use the current year or match nearby conventions.
📝 Suggested fix
-// Copyright 2023 PingCAP, Inc. +// Copyright 2025 PingCAP, Inc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ddl/backfilling_dist_scheduler_internal_test.go` around lines 1 - 13, Update the license header year in the new test file backfilling_dist_scheduler_internal_test.go to the current year (2026) or match the surrounding project's convention; open the file header comment block and replace "Copyright 2023" with the correct year so the file's license metadata is accurate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/ddl/backfilling_dist_scheduler_internal_test.go`:
- Around line 1-13: Update the license header year in the new test file
backfilling_dist_scheduler_internal_test.go to the current year (2026) or match
the surrounding project's convention; open the file header comment block and
replace "Copyright 2023" with the correct year so the file's license metadata is
accurate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 987b71ae-0935-4d24-a033-8de66a3c9875
📒 Files selected for processing (6)
pkg/ddl/backfilling_dist_scheduler.gopkg/ddl/backfilling_dist_scheduler_internal_test.gopkg/ddl/backfilling_dist_scheduler_test.gopkg/lightning/backend/external/split.gopkg/lightning/backend/external/split_test.gopkg/tici/tici.proto
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/ddl/backfilling_dist_scheduler_test.go
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: GMHDBJD, OliverS929, wjhuang2016 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 |
|
/test pull-unit-test-ddlv1 |
|
/test fast_test_tiprow_for_release |
|
@3pointer: The specified target(s) for The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions 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. |
|
@3pointer: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions 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. |
|
@3pointer: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions 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. |
|
/test pull-br-integration-test |
|
@3pointer: PRs from untrusted users cannot be marked as trusted with DetailsIn response to this:
Instructions 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. |
|
/ok-to-test |
|
/test pull-integration-tici-test |
|
/test pull-br-integration-test |
|
@3pointer: The specified target(s) for Use DetailsIn response to this:
Instructions 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. |
|
@3pointer: The specified target(s) for Use DetailsIn response to this:
Instructions 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. |
|
/test fast_test_tiprow_for_release |
|
@3pointer: The specified target(s) for The following commands are available to trigger optional jobs: Use DetailsIn response to this:
Instructions 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. |
|
@3pointer: The following test 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. |
4 participants
What problem does this PR solve?
Issue Number: close #xxx
Problem Summary:
When TiDB builds FULLTEXT or HYBRID indexes with the TiCI backend in global sort mode, TiCI does not get an early pre-split signal before the write-and-ingest stage starts. As a result, TiCI cannot use the aggregated SortedKV metadata to analyze shard distribution and split internal shards ahead of ingest.
In addition, the TiDB side does not define the corresponding
PreSplitImportShardsRPC yet, so the pre-split request cannot be sent with the required metadata.What changed and how does it work?
This PR is scoped to DDL backfill only (ADD FULLTEXT INDEX / ADD HYBRID INDEX). IMPORT INTO is not supported in this PR; follow-up changes can be made separately if needed.
This PR adds a TiCI pre-split hook in
generateGlobalSortIngestPlanfor TiCI-backed FULLTEXT and HYBRID index jobs.The main changes are:
PreSplitImportShardsRPC and related request/response messages topkg/tici/tici.proto, then regeneratepkg/tici/tici.pb.go.pkg/tici/tici_manager_client.goto callPreSplitImportShards, including keyspace propagation and test failpoint hooks.generateGlobalSortIngestPlan, detectActionAddFullTextIndexandActionAddHybridIndex, aggregate mergedSortedKVMetagroups, and build a TiCI pre-split request with:start_key/end_keytotal_kv_size/total_kv_cntdata_file_count/stat_file_countmeta_groupsCheck List
Tests
Unit test
Suggested / used test commands:
make failpoint-enable && (cd pkg/ddl && go test -run TestBackfillingSchedulerGlobalSortModeTiCIPreSplit --tags=intest; rc=$?; cd ../..; make failpoint-disable; exit $rc)go test -run TestPreSplitImportShards --tags=intest ./pkg/ticigo test -run TestPreSplitImportShardsMock --tags=intest ./pkg/ticiIntegration test
Manual test (add detailed scripts or steps below)
No need to test
Side effects
Documentation
Release note