*: use task key in TiCI requests | tidb-test=13ccf8de48e8db2290ff884598444d0508606bbf tiflash=feature-fts#67786
Conversation
ti-chi-bot
bot
added
do-not-merge/invalid-title
release-note-none
|
Hi @OliverS929. 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. |
📝 WalkthroughWalkthroughReplaces numeric job.ID string usage with canonical task-key-based TiDB task IDs across DXF import-into, importer, DDL, and TiCI wiring; adds helpers and fields to propagate TiDB-side task identifiers and extends test coverage and TiCI test instrumentation. Changes
Sequence Diagram(s)sequenceDiagram
participant DXF as DXF Executor
participant TaskMeta as TaskMeta/DDL Job
participant Ctrl as LoadDataController
participant Importer as TableImporter
participant Backend as Local Backend / TiCI Client
participant TiCI as TiCI Service
DXF->>TaskMeta: read JobID
DXF->>DXF: compute tidbTaskID = ticiTaskIDFor*(JobID)
DXF->>Ctrl: InitDataStore(...) then set TiDBTaskIDForTiCI=tidbTaskID
DXF->>Importer: NewTableImporter(controller)
Importer->>Importer: resolve tidbTaskID = ticiTaskIDForImporter(controller, importerID)
Importer->>Backend: InitTiCIWriterGroup(tidbTaskID, ...)
Backend->>TiCI: send Init/Create requests with TidbTaskId
TiCI-->>Backend: responses
Backend-->>Importer: writer group ready
Importer-->>DXF: importer ready
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/dxf/importinto/task_executor_test.go (1)
137-139: Prefer unified importer teardown in test cleanup.Optional: use
tableImporter.Close()(viat.Cleanup) instead of manually closing controller/backend pieces, to keep teardown aligned with production lifecycle.♻️ Optional cleanup refactor
- tableImporter.LoadDataController.Close() - tableImporter.Backend().CloseEngineMgr() + t.Cleanup(func() { + require.NoError(t, tableImporter.Close()) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/dxf/importinto/task_executor_test.go` around lines 137 - 139, The test manually closes components via tableImporter.LoadDataController.Close() and tableImporter.Backend().CloseEngineMgr(); instead, call the unified teardown method tableImporter.Close() and register it with t.Cleanup (e.g., t.Cleanup(tableImporter.Close)) so the importer is torn down the same way as production and cleanup runs automatically even on test failure.
🤖 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/dxf/importinto/task_executor_test.go`:
- Around line 137-139: The test manually closes components via
tableImporter.LoadDataController.Close() and
tableImporter.Backend().CloseEngineMgr(); instead, call the unified teardown
method tableImporter.Close() and register it with t.Cleanup (e.g.,
t.Cleanup(tableImporter.Close)) so the importer is torn down the same way as
production and cleanup runs automatically even on test failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4623ce1b-2f2c-4297-8f21-641b7a9714a2
📒 Files selected for processing (6)
pkg/dxf/importinto/subtask_executor.gopkg/dxf/importinto/task_executor.gopkg/dxf/importinto/task_executor_test.gopkg/executor/importer/import.gopkg/executor/importer/table_import.gopkg/executor/importer/table_import_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/fts #67786 +/- ##
===================================================
- Coverage 76.8610% 75.5780% -1.2831%
===================================================
Files 1960 1945 -15
Lines 555677 558759 +3082
===================================================
- Hits 427099 422299 -4800
- Misses 127116 136018 +8902
+ Partials 1462 442 -1020
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
OliverS929
changed the title
ti-chi-bot
bot
added
size/XL
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/tici/tici_manager_client_test.go (1)
145-176: Optional: extract the repeatedGetImportStoragePrefixmatcher into one local helper.This keeps the test easier to maintain while preserving the stronger request validation you added.
♻️ Suggested cleanup
func TestGetImportStoragePrefix(t *testing.T) { @@ - mockClient. - On("GetImportStoragePrefix", mock.Anything, mock.MatchedBy(func(req *GetImportStoragePrefixRequest) bool { - return matchKeyspace[*GetImportStoragePrefixRequest](keyspaceID)(req) && - req.GetTidbTaskId() == taskID && - req.GetTableId() == tblID && - slices.Equal(req.GetIndexIds(), indexIDs) - })). + matchReq := func(req *GetImportStoragePrefixRequest) bool { + return matchKeyspace[*GetImportStoragePrefixRequest](keyspaceID)(req) && + req.GetTidbTaskId() == taskID && + req.GetTableId() == tblID && + slices.Equal(req.GetIndexIds(), indexIDs) + } + + mockClient. + On("GetImportStoragePrefix", mock.Anything, mock.MatchedBy(matchReq)). Return(&GetImportStoragePrefixResponse{Status: ErrorCode_SUCCESS, JobId: 100, StorageUri: "/s3/path?endpoint=http://127.0.0.1"}, nil). Once() @@ - mockClient. - On("GetImportStoragePrefix", mock.Anything, mock.MatchedBy(func(req *GetImportStoragePrefixRequest) bool { - return matchKeyspace[*GetImportStoragePrefixRequest](keyspaceID)(req) && - req.GetTidbTaskId() == taskID && - req.GetTableId() == tblID && - slices.Equal(req.GetIndexIds(), indexIDs) - })). + mockClient. + On("GetImportStoragePrefix", mock.Anything, mock.MatchedBy(matchReq)). @@ - mockClient. - On("GetImportStoragePrefix", mock.Anything, mock.MatchedBy(func(req *GetImportStoragePrefixRequest) bool { - return matchKeyspace[*GetImportStoragePrefixRequest](keyspaceID)(req) && - req.GetTidbTaskId() == taskID && - req.GetTableId() == tblID && - slices.Equal(req.GetIndexIds(), indexIDs) - })). + mockClient. + On("GetImportStoragePrefix", mock.Anything, mock.MatchedBy(matchReq)).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tici/tici_manager_client_test.go` around lines 145 - 176, The test repeats the same mock.MatchedBy matcher for GetImportStoragePrefix calls; extract that matcher into a local variable (e.g., importPrefixMatcher or getImportStoragePrefixMatcher) defined once in tici_manager_client_test.go and reuse it in each mockClient.On call and any subsequent expectations; ensure the matcher closure still references keyspaceID, taskID, tblID, and indexIDs and returns the same bool check (using matchKeyspace[*GetImportStoragePrefixRequest](keyspaceID)(req) && req.GetTidbTaskId() == taskID && req.GetTableId() == tblID && slices.Equal(req.GetIndexIds(), indexIDs)) so behavior and validation remain identical.
🤖 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/index_modify_test.go`:
- Around line 1992-1995: Add an explicit negative assertion that
GetImportStoragePrefix was not called: after capturing raw from
tici.GetMockTiCIFinishIndexUploadRequest() (the variable named raw in this test
and the mock method GetImportStoragePrefix), assert that the captured request
for GetImportStoragePrefix is empty/nil (or that the mock recorded zero calls)
so the test fails if GetImportStoragePrefix is invoked; update the test around
tici.GetMockTiCIFinishIndexUploadRequest() / raw to include that single
deterministic assertion and nothing else.
---
Nitpick comments:
In `@pkg/tici/tici_manager_client_test.go`:
- Around line 145-176: The test repeats the same mock.MatchedBy matcher for
GetImportStoragePrefix calls; extract that matcher into a local variable (e.g.,
importPrefixMatcher or getImportStoragePrefixMatcher) defined once in
tici_manager_client_test.go and reuse it in each mockClient.On call and any
subsequent expectations; ensure the matcher closure still references keyspaceID,
taskID, tblID, and indexIDs and returns the same bool check (using
matchKeyspace[*GetImportStoragePrefixRequest](keyspaceID)(req) &&
req.GetTidbTaskId() == taskID && req.GetTableId() == tblID &&
slices.Equal(req.GetIndexIds(), indexIDs)) so behavior and validation remain
identical.
🪄 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: 530cbe2b-970a-4803-912b-5097a7fd708b
📒 Files selected for processing (13)
pkg/ddl/backfilling_dist_scheduler.gopkg/ddl/backfilling_dist_scheduler_test.gopkg/ddl/backfilling_import_cloud.gopkg/ddl/ddl_test.gopkg/ddl/index.gopkg/ddl/index_modify_test.gopkg/ddl/ingest/engine_mgr.gopkg/ddl/util/BUILD.bazelpkg/ddl/util/util.gopkg/tici/tici_manager_client.gopkg/tici/tici_manager_client_test.gopkg/tici/tici_write.gopkg/tici/tici_write_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/ddl/util/BUILD.bazel
- pkg/ddl/backfilling_dist_scheduler.go
| // Empty-table fulltext add-index skips reorg/backfill, so TiCI upload finalization is | ||
| // still exercised here but GetImportStoragePrefix is not. | ||
| raw = tici.GetMockTiCIFinishIndexUploadRequest() | ||
| require.NotEmpty(t, raw) |
There was a problem hiding this comment.
Add an explicit negative assertion for GetImportStoragePrefix.
The comment says this path should not call GetImportStoragePrefix, but the test never checks it. Please assert the captured request is empty to prevent silent regressions.
Suggested patch
// Empty-table fulltext add-index skips reorg/backfill, so TiCI upload finalization is
// still exercised here but GetImportStoragePrefix is not.
+ raw = tici.GetMockTiCIGetImportStoragePrefixRequest()
+ require.Empty(t, raw)
+
raw = tici.GetMockTiCIFinishIndexUploadRequest()
require.NotEmpty(t, raw)As per coding guidelines, "Keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Empty-table fulltext add-index skips reorg/backfill, so TiCI upload finalization is | |
| // still exercised here but GetImportStoragePrefix is not. | |
| raw = tici.GetMockTiCIFinishIndexUploadRequest() | |
| require.NotEmpty(t, raw) | |
| // Empty-table fulltext add-index skips reorg/backfill, so TiCI upload finalization is | |
| // still exercised here but GetImportStoragePrefix is not. | |
| raw = tici.GetMockTiCIGetImportStoragePrefixRequest() | |
| require.Empty(t, raw) | |
| raw = tici.GetMockTiCIFinishIndexUploadRequest() | |
| require.NotEmpty(t, raw) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/ddl/index_modify_test.go` around lines 1992 - 1995, Add an explicit
negative assertion that GetImportStoragePrefix was not called: after capturing
raw from tici.GetMockTiCIFinishIndexUploadRequest() (the variable named raw in
this test and the mock method GetImportStoragePrefix), assert that the captured
request for GetImportStoragePrefix is empty/nil (or that the mock recorded zero
calls) so the test fails if GetImportStoragePrefix is invoked; update the test
around tici.GetMockTiCIFinishIndexUploadRequest() / raw to include that single
deterministic assertion and nothing else.
|
/retest |
|
@OliverS929: 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. |
|
/retest |
|
@OliverS929: 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. |
|
/retest |
|
@OliverS929: 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. |
|
/retest |
|
@OliverS929: 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. |
| ) | ||
|
|
||
| // BuildBackfillTaskKey generates the canonical dist-task key for a DDL backfill job. | ||
| func BuildBackfillTaskKey(jobID int64, mergeTempIdx bool) string { |
There was a problem hiding this comment.
if that's only for tici.
Can we name it more Coherent with tici index, something like AddIndex/1, ImportInto/1
There was a problem hiding this comment.
This is not only for TiCI, but a broader rewrite of the generic task key generation logic for DDL, with the goal of minimizing additional imports and package dependencies.
|
/test pull_br_integration_test |
OliverS929
changed the title
|
@OliverS929: 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. |
|
@OliverS929: 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 |
|
@OliverS929: 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. |
|
@OliverS929: 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. |
OliverS929
changed the title
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wjhuang2016 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 |
[LGTM Timeline notifier]Timeline:
|
3 participants
What problem does this PR solve?
Issue Number: ref #63937
Problem Summary:
TiCI needs the full TiDB task key instead of only the numeric id, so it can identify task type and query task status for GC.
What changed and how does it work?
This PR updates the TiDB -> TiCI task identifier wiring for both IMPORT INTO and DDL TiCI paths.
GetImportStoragePrefix,FinishIndexUpload, and DDL global-sort pre-split requests.Check List
Tests
Executed:
make failpoint-enable && ( go test -run 'TestTiCITaskIDForImportIntoUsesTaskKey|TestGetTableImporterSetsTiCITaskID' --tags=intest ./pkg/dxf/importinto && go test -run 'TestTiCITaskIDForImporter|TestCheckRequirementsWithTiCIIndexLocalSort' --tags=intest ./pkg/executor/importer ); rc=$?; make failpoint-disable; exit $rcmake failpoint-enable && ( go test -run 'TestNewTiCIDataWriterGroupUsesInjectedManagerFactory|TestGetImportStoragePrefix|TestFinishIndexUploadHelper' --tags=intest ./pkg/tici ); rc=$?; make failpoint-disable; exit $rcmake failpoint-enable && ( go test -run 'TestFullTextIndexSysvarsPassedToTiCI|TestBackfillingSchedulerGlobalSortModeTiCIPreSplit|TestTiCITaskIDForDDLUsesTaskKey' --tags=intest ./pkg/ddl ); rc=$?; make failpoint-disable; exit $rcSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Refactor
Tests