pkg/ddl, pkg/tici: add TiCI pre-split for DDL global sort ingest | tidb-test=13ccf8de48e8db2290ff884598444d0508606bbf tiflash=feature-fts#67313
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
bot
added
do-not-merge/work-in-progress
📝 WalkthroughWalkthroughThe changes integrate a new TiCI PreSplitImportShards RPC into the DDL backfilling scheduler to support pre-splitting import shards before full index building. The RangeSplitter API is extended to return group size and key count metrics, enabling accurate tracking of split groups. TiCI client code is added with failpoint test support, and DDL backfilling logic now conditionally invokes TiCI pre-split for full-text and hybrid index jobs. Changes
Sequence DiagramsequenceDiagram
participant Scheduler as DDL Backfilling<br/>Scheduler
participant Storage as Storage<br/>(PD + Codec)
participant RangeSplit as RangeSplitter
participant TiCI as TiCI Client
participant MetaService as TiCI Meta<br/>Service
Scheduler->>Storage: validateStorage<br/>as storageWithPDAndCodec
Storage-->>Scheduler: ✓ codec available
Scheduler->>RangeSplit: SplitOneRangesGroup()
RangeSplit-->>Scheduler: endKey, dataFiles,<br/>groupSize, groupKeyCnt
Scheduler->>Scheduler: Aggregate groups<br/>to 1GiB report groups<br/>Deduplicate file counts
Scheduler->>TiCI: buildTiCIPreSplitRequest<br/>(task, table, index IDs,<br/>aggregated KV stats,<br/>report groups)
TiCI->>MetaService: PreSplitImportShards<br/>Request (timeout: 1min)
MetaService-->>TiCI: Response (split_keys,<br/>shard_counts)
TiCI-->>Scheduler: ✓ pre-split complete<br/>or log error & continue
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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
ti-chi-bot
bot
removed
the
do-not-merge/work-in-progress
|
/ok-to-test |
ti-chi-bot
bot
added
approved
needs-1-more-lgtm
![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)
pkg/tici/tici.proto (1)
600-629: Document thestart_key/end_keycontract.
KeyRangeabove explicitly defines its bounds, but these new messages do not. Since the scheduler is feeding split boundaries straight into this RPC, it would help to state whetherend_keyis exclusive so TiCI does not have to infer it.Proposed comment update
// One merged SortedKVMeta group used for import pre-split analysis. message PreSplitImportShardMeta { int64 ele_id = 1; + // Inclusive lower bound of this meta group. bytes start_key = 2; + // Exclusive upper bound of this meta group. bytes end_key = 3; uint64 total_kv_size = 4; uint64 total_kv_cnt = 5; int32 data_file_count = 6; int32 stat_file_count = 7; } message PreSplitImportShardsRequest { // TiDB unique task ID for this Import Into/Index Backfilling job. string tidb_task_id = 1; // Table ID of the target table. int64 table_id = 2; // Index ID of the target index. If this is an Import Into job that relates // to multiple indexes, this field should contain all the index IDs. repeated int64 index_ids = 3; uint64 scan_snapshot_ts = 4; + // Inclusive lower bound across all meta_groups. bytes start_key = 5; + // Exclusive upper bound across all meta_groups. bytes end_key = 6; uint64 total_kv_size = 7;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 the current code and only fix it if needed. In `@pkg/tici/tici.proto` around lines 600 - 629, The proto messages PreSplitImportShardMeta and PreSplitImportShardsRequest lack a clear contract for the start_key/end_key semantics; update their comments to state the exact bounds (e.g., whether start_key is inclusive and end_key is exclusive, how empty/null keys are treated, and any required prefix/encoding assumptions) so callers (and TiCI scheduler) don't have to infer behavior from KeyRange; add this clarifying text to the comments above the start_key/end_key fields in both PreSplitImportShardMeta and PreSplitImportShardsRequest and reference KeyRange only to note consistency with its inclusive/exclusive convention.
🤖 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/tici/tici_manager_client.go`:
- Around line 431-439: The mock interception call maybeMockPreSplitImportShards
is invoked before the request is enriched with KeyspaceId, so tests that
exercise the mock path see a different payload than the real RPC; move the
maybeMockPreSplitImportShards(req) invocation to after the request is
normalized/enriched (i.e., after req.KeyspaceId = t.getKeyspaceID()) in the
PreSplitImportShards method (and the other similar entry point referenced),
ensuring ManagerCtx.getKeyspaceID() is applied before calling
maybeMockPreSplitImportShards so the mock sees the same request as the real RPC.
---
Nitpick comments:
In `@pkg/tici/tici.proto`:
- Around line 600-629: The proto messages PreSplitImportShardMeta and
PreSplitImportShardsRequest lack a clear contract for the start_key/end_key
semantics; update their comments to state the exact bounds (e.g., whether
start_key is inclusive and end_key is exclusive, how empty/null keys are
treated, and any required prefix/encoding assumptions) so callers (and TiCI
scheduler) don't have to infer behavior from KeyRange; add this clarifying text
to the comments above the start_key/end_key fields in both
PreSplitImportShardMeta and PreSplitImportShardsRequest and reference KeyRange
only to note consistency with its inclusive/exclusive convention.
🪄 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: 11723cfd-b5c0-4d14-a299-6f6b509f63e1
⛔ Files ignored due to path filters (1)
pkg/tici/tici.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (13)
pkg/ddl/BUILD.bazelpkg/ddl/backfilling_dist_scheduler.gopkg/ddl/backfilling_dist_scheduler_internal_test.gopkg/ddl/backfilling_dist_scheduler_test.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/BUILD.bazelpkg/tici/tici.protopkg/tici/tici_manager_client.gopkg/tici/tici_manager_client_test.go
| func (t *ManagerCtx) PreSplitImportShards(ctx context.Context, req *PreSplitImportShardsRequest) error { | ||
| if handled, err := maybeMockPreSplitImportShards(req); handled { | ||
| return err | ||
| } | ||
| if req == nil { | ||
| return errors.New("pre split import shards request is nil") | ||
| } | ||
| req.KeyspaceId = t.getKeyspaceID() | ||
|
|
There was a problem hiding this comment.
Move the mock interception after request enrichment.
Both entry points run maybeMockPreSplitImportShards(req) before req.KeyspaceId is filled, so the captured payload in the new failpoint-based tests is not the same request that the real RPC sends. That makes the mock path blind to keyspace-propagation regressions.
Suggested fix
func (t *ManagerCtx) PreSplitImportShards(ctx context.Context, req *PreSplitImportShardsRequest) error {
- if handled, err := maybeMockPreSplitImportShards(req); handled {
- return err
- }
if req == nil {
return errors.New("pre split import shards request is nil")
}
req.KeyspaceId = t.getKeyspaceID()
+ if handled, err := maybeMockPreSplitImportShards(req); handled {
+ return err
+ }
t.mu.RLock()
defer t.mu.RUnlock()
...
} func PreSplitImportShards(ctx context.Context, store keyspaceStorage, req *PreSplitImportShardsRequest) error {
- if handled, err := maybeMockPreSplitImportShards(req); handled {
- return err
- }
+ if req == nil {
+ return errors.New("pre split import shards request is nil")
+ }
+ if store != nil {
+ req.KeyspaceId = uint32(store.GetCodec().GetKeyspaceID())
+ }
+ if handled, err := maybeMockPreSplitImportShards(req); handled {
+ return err
+ }
etcdClient, err := getEtcdClientFunc()
...
}Also applies to: 1038-1060
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/tici/tici_manager_client.go` around lines 431 - 439, The mock
interception call maybeMockPreSplitImportShards is invoked before the request is
enriched with KeyspaceId, so tests that exercise the mock path see a different
payload than the real RPC; move the maybeMockPreSplitImportShards(req)
invocation to after the request is normalized/enriched (i.e., after
req.KeyspaceId = t.getKeyspaceID()) in the PreSplitImportShards method (and the
other similar entry point referenced), ensuring ManagerCtx.getKeyspaceID() is
applied before calling maybeMockPreSplitImportShards so the mock sees the same
request as the real RPC.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-fts-202602 #67313 +/- ##
=======================================================
Coverage ? 76.7314%
=======================================================
Files ? 1962
Lines ? 558695
Branches ? 0
=======================================================
Hits ? 428695
Misses ? 128538
Partials ? 1462
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/retest |
3pointer
changed the title
|
/retest |
|
/test pull-error-log-review |
|
@3pointer: No presubmit jobs available for pingcap/tidb@release-fts-202602 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: GMHDBJD, 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 |
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
@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. |
ti-chi-bot
bot
merged commit 2338f7d
into
pingcap:release-fts-202602
3 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