Repository navigation
localbackend: move to ingestctrl and rename package - #68382
ti-chi-bot[bot] merged 9 commits into
Conversation
ti-chi-bot
Bot
added
the
release-note-none
|
@D3Hunter 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/XL
|
Hi @D3Hunter. 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:
📝 WalkthroughWalkthroughRepository-wide migration replacing usages of pkg/lightning/backend/local with pkg/ingestor/ingestctrl: package renames, imports and types updated, BUILD deps and Makefile/mocks adjusted, nogo exemptions changed, and test failpoints/scripts rewired. ChangesIngest Package Reorganization
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
D3Hunter
changed the title
ti-chi-bot
Bot
added
size/XXL
![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
🤖 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 `@lightning/pkg/importer/checksum_helper.go`:
- Line 23: The import of "github.com/pingcap/tidb/pkg/ingestor/ingestctrl" must
be aliased to local because the file uses symbols like local.ChecksumManager,
local.NewTiKVChecksumManager, local.NewTiDBChecksumExecutor, and other local.*
references; update the import line to add the alias `local` so those references
resolve (e.g., change the import to use `local` as the package name for
ingestctrl).
In `@lightning/pkg/server/lightning_serial_test.go`:
- Around line 167-185: The test enables failpoints but only disables
GetRlimitValue conditionally, which can leak if a require assertion aborts the
test; immediately call defer (or t.Cleanup) to disable each failpoint right
after calling failpoint.Enable for
"github.com/pingcap/tidb/pkg/ingestor/ingestctrl/GetRlimitValue" and for
".../SetRlimitError" so cleanup is guaranteed even on early return — update the
test around the calls to failpoint.Enable (and the subsequent checks that call
checkSystemRequirement) to install defer failpoint.Disable(...) (or
t.Cleanup(func(){ failpoint.Disable(...) })) immediately after each Enable call.
🪄 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: 8855d905-22f9-432a-bc68-dba46e653f57
📒 Files selected for processing (89)
Makefilebr/pkg/mock/mocklocal/local.gobuild/nogo_config.jsonlightning/pkg/importer/BUILD.bazellightning/pkg/importer/checksum_helper.golightning/pkg/importer/get_pre_info.golightning/pkg/importer/import.golightning/pkg/importer/meta_manager_test.golightning/pkg/importer/table_import.golightning/pkg/importer/table_import_test.golightning/pkg/server/BUILD.bazellightning/pkg/server/checkpoint_control.golightning/pkg/server/lightning.golightning/pkg/server/lightning_serial_test.golightning/tests/lightning_checkpoint_chunks/run.shlightning/tests/lightning_concurrent-restore/run.shlightning/tests/lightning_disable_scheduler_by_key_range/run.shlightning/tests/lightning_fail_fast_on_nonretry_err/run.shlightning/tests/lightning_import_compress/run.shlightning/tests/lightning_local_backend/run.shlightning/tests/lightning_max_random/run.shlightning/tests/lightning_write_batch/run.shlightning/tests/lightning_write_timeout/run.shpkg/ddl/BUILD.bazelpkg/ddl/backfilling.gopkg/ddl/backfilling_dist_scheduler.gopkg/ddl/backfilling_import_cloud.gopkg/ddl/backfilling_read_index.gopkg/ddl/index.gopkg/ddl/ingest/BUILD.bazelpkg/ddl/ingest/backend.gopkg/ddl/ingest/backend_mgr.gopkg/ddl/ingest/config.gopkg/ddl/ingest/mock.gopkg/domain/BUILD.bazelpkg/domain/domain.gopkg/dxf/importinto/BUILD.bazelpkg/dxf/importinto/subtask_executor.gopkg/executor/importer/BUILD.bazelpkg/executor/importer/import.gopkg/executor/importer/importer_testkit_test.gopkg/executor/importer/table_import.gopkg/ingestor/ingestctrl/BUILD.bazelpkg/ingestor/ingestctrl/checksum.gopkg/ingestor/ingestctrl/checksum_test.gopkg/ingestor/ingestctrl/compress.gopkg/ingestor/ingestctrl/compress_test.gopkg/ingestor/ingestctrl/disk_quota.gopkg/ingestor/ingestctrl/disk_quota_test.gopkg/ingestor/ingestctrl/duplicate.gopkg/ingestor/ingestctrl/duplicate_test.gopkg/ingestor/ingestctrl/engine.gopkg/ingestor/ingestctrl/engine_mgr.gopkg/ingestor/ingestctrl/engine_mgr_test.gopkg/ingestor/ingestctrl/engine_test.gopkg/ingestor/ingestctrl/iterator.gopkg/ingestor/ingestctrl/iterator_test.gopkg/ingestor/ingestctrl/job_worker.gopkg/ingestor/ingestctrl/job_worker_test.gopkg/ingestor/ingestctrl/local.gopkg/ingestor/ingestctrl/local_check_test.gopkg/ingestor/ingestctrl/local_freebsd.gopkg/ingestor/ingestctrl/local_test.gopkg/ingestor/ingestctrl/local_unix.gopkg/ingestor/ingestctrl/local_unix_generic.gopkg/ingestor/ingestctrl/local_windows.gopkg/ingestor/ingestctrl/localhelper.gopkg/ingestor/ingestctrl/localhelper_test.gopkg/ingestor/ingestctrl/main_test.gopkg/ingestor/ingestctrl/rate_limiter.gopkg/ingestor/ingestctrl/rate_limiter_param.gopkg/ingestor/ingestctrl/rate_limiter_test.gopkg/ingestor/ingestctrl/region_job.gopkg/ingestor/ingestctrl/region_job_test.gopkg/ingestor/ingestctrl/tikv_mode.gopkg/server/handler/tikvhandler/BUILD.bazelpkg/server/handler/tikvhandler/tikv_handler.gotests/realtikvtest/addindextest/add_index_test.gotests/realtikvtest/addindextest1/BUILD.bazeltests/realtikvtest/addindextest1/disttask_test.gotests/realtikvtest/addindextest2/BUILD.bazeltests/realtikvtest/addindextest2/alter_job_test.gotests/realtikvtest/addindextest2/global_sort_test.gotests/realtikvtest/addindextest3/BUILD.bazeltests/realtikvtest/addindextest3/ingest_test.gotests/realtikvtest/importintotest/BUILD.bazeltests/realtikvtest/importintotest/import_into_test.gotests/realtikvtest/importintotest2/from_select_test.gotests/realtikvtest/importintotest4/global_sort_test.go
| err := failpoint.Enable("github.com/pingcap/tidb/pkg/ingestor/ingestctrl/GetRlimitValue", "return(139415)") | ||
| require.NoError(t, err) | ||
| err = failpoint.Enable("github.com/pingcap/tidb/pkg/lightning/backend/local/SetRlimitError", "return(true)") | ||
| err = failpoint.Enable("github.com/pingcap/tidb/pkg/ingestor/ingestctrl/SetRlimitError", "return(true)") | ||
| require.NoError(t, err) | ||
| defer func() { | ||
| _ = failpoint.Disable("github.com/pingcap/tidb/pkg/lightning/backend/local/SetRlimitError") | ||
| _ = failpoint.Disable("github.com/pingcap/tidb/pkg/ingestor/ingestctrl/SetRlimitError") | ||
| }() | ||
| // with this dbMetas, the estimated fds will be 139416, so should return error | ||
| err = checkSystemRequirement(cfg, dbMetas) | ||
| require.Error(t, err) | ||
|
|
||
| err = failpoint.Disable("github.com/pingcap/tidb/pkg/lightning/backend/local/GetRlimitValue") | ||
| err = failpoint.Disable("github.com/pingcap/tidb/pkg/ingestor/ingestctrl/GetRlimitValue") | ||
| require.NoError(t, err) | ||
|
|
||
| // the min rlimit should be not smaller than the default min value (139416) | ||
| err = failpoint.Enable("github.com/pingcap/tidb/pkg/lightning/backend/local/GetRlimitValue", "return(139416)") | ||
| err = failpoint.Enable("github.com/pingcap/tidb/pkg/ingestor/ingestctrl/GetRlimitValue", "return(139416)") | ||
| defer func() { | ||
| _ = failpoint.Disable("github.com/pingcap/tidb/pkg/lightning/backend/local/GetRlimitValue") | ||
| _ = failpoint.Disable("github.com/pingcap/tidb/pkg/ingestor/ingestctrl/GetRlimitValue") | ||
| }() |
There was a problem hiding this comment.
Ensure failpoint cleanup is guaranteed even on early test failure.
GetRlimitValue is enabled on Line 167, but cleanup before re-enable depends on reaching Line 178. If a require.* aborts earlier, the failpoint can leak to subsequent tests.
Suggested fix
err := failpoint.Enable("github.com/pingcap/tidb/pkg/ingestor/ingestctrl/GetRlimitValue", "return(139415)")
require.NoError(t, err)
+defer func() {
+ _ = failpoint.Disable("github.com/pingcap/tidb/pkg/ingestor/ingestctrl/GetRlimitValue")
+}()
err = failpoint.Enable("github.com/pingcap/tidb/pkg/ingestor/ingestctrl/SetRlimitError", "return(true)")
require.NoError(t, err)As per coding guidelines, "Unit tests in a package that uses failpoints: MUST enable failpoints before tests and disable afterward."
📝 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.
| err := failpoint.Enable("github.com/pingcap/tidb/pkg/ingestor/ingestctrl/GetRlimitValue", "return(139415)") | |
| require.NoError(t, err) | |
| err = failpoint.Enable("github.com/pingcap/tidb/pkg/lightning/backend/local/SetRlimitError", "return(true)") | |
| err = failpoint.Enable("github.com/pingcap/tidb/pkg/ingestor/ingestctrl/SetRlimitError", "return(true)") | |
| require.NoError(t, err) | |
| defer func() { | |
| _ = failpoint.Disable("github.com/pingcap/tidb/pkg/lightning/backend/local/SetRlimitError") | |
| _ = failpoint.Disable("github.com/pingcap/tidb/pkg/ingestor/ingestctrl/SetRlimitError") | |
| }() | |
| // with this dbMetas, the estimated fds will be 139416, so should return error | |
| err = checkSystemRequirement(cfg, dbMetas) | |
| require.Error(t, err) | |
| err = failpoint.Disable("github.com/pingcap/tidb/pkg/lightning/backend/local/GetRlimitValue") | |
| err = failpoint.Disable("github.com/pingcap/tidb/pkg/ingestor/ingestctrl/GetRlimitValue") | |
| require.NoError(t, err) | |
| // the min rlimit should be not smaller than the default min value (139416) | |
| err = failpoint.Enable("github.com/pingcap/tidb/pkg/lightning/backend/local/GetRlimitValue", "return(139416)") | |
| err = failpoint.Enable("github.com/pingcap/tidb/pkg/ingestor/ingestctrl/GetRlimitValue", "return(139416)") | |
| defer func() { | |
| _ = failpoint.Disable("github.com/pingcap/tidb/pkg/lightning/backend/local/GetRlimitValue") | |
| _ = failpoint.Disable("github.com/pingcap/tidb/pkg/ingestor/ingestctrl/GetRlimitValue") | |
| }() | |
| err := failpoint.Enable("github.com/pingcap/tidb/pkg/ingestor/ingestctrl/GetRlimitValue", "return(139415)") | |
| require.NoError(t, err) | |
| defer func() { | |
| _ = failpoint.Disable("github.com/pingcap/tidb/pkg/ingestor/ingestctrl/GetRlimitValue") | |
| }() | |
| err = failpoint.Enable("github.com/pingcap/tidb/pkg/ingestor/ingestctrl/SetRlimitError", "return(true)") | |
| require.NoError(t, err) | |
| defer func() { | |
| _ = failpoint.Disable("github.com/pingcap/tidb/pkg/ingestor/ingestctrl/SetRlimitError") | |
| }() | |
| // with this dbMetas, the estimated fds will be 139416, so should return error | |
| err = checkSystemRequirement(cfg, dbMetas) | |
| require.Error(t, err) | |
| err = failpoint.Disable("github.com/pingcap/tidb/pkg/ingestor/ingestctrl/GetRlimitValue") | |
| require.NoError(t, err) | |
| // the min rlimit should be not smaller than the default min value (139416) | |
| err = failpoint.Enable("github.com/pingcap/tidb/pkg/ingestor/ingestctrl/GetRlimitValue", "return(139416)") | |
| defer func() { | |
| _ = failpoint.Disable("github.com/pingcap/tidb/pkg/ingestor/ingestctrl/GetRlimitValue") | |
| }() |
🤖 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 `@lightning/pkg/server/lightning_serial_test.go` around lines 167 - 185, The
test enables failpoints but only disables GetRlimitValue conditionally, which
can leak if a require assertion aborts the test; immediately call defer (or
t.Cleanup) to disable each failpoint right after calling failpoint.Enable for
"github.com/pingcap/tidb/pkg/ingestor/ingestctrl/GetRlimitValue" and for
".../SetRlimitError" so cleanup is guaranteed even on early return — update the
test around the calls to failpoint.Enable (and the subsequent checks that call
checkSystemRequirement) to install defer failpoint.Disable(...) (or
t.Cleanup(func(){ failpoint.Disable(...) })) immediately after each Enable call.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68382 +/- ##
================================================
- Coverage 77.2971% 77.2327% -0.0644%
================================================
Files 2010 2036 +26
Lines 555568 569692 +14124
================================================
+ Hits 429438 439989 +10551
- Misses 125210 127767 +2557
- Partials 920 1936 +1016
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
D3Hunter
changed the title
|
🔍 Starting code review for this PR... |
ingress-bot
left a comment
ingress-bot
left a comment
There was a problem hiding this comment.
This review was generated by AI and should be verified by a human reviewer.
Manual follow-up is recommended before merge.
Summary
- Total findings: 0
- Inline comments: 0
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
No findings.
|
/retest |
|
@D3Hunter: 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. |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: GMHDBJD, joechenrh, Leavrth 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 |
|
/retest |
|
@D3Hunter: 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 |
|
@D3Hunter: 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 |
|
@D3Hunter: 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. |
5 participants
What problem does this PR solve?
Issue Number: ref #68346
Problem Summary:
The local ingest backend is being moved from
pkg/lightning/backend/localtopkg/ingestor/ingestctrl.After path migration, the Go package name was still
local, which is inconsistent with the new directory/package identity.What changed and how does it work?
Compared with
master, this PR now contains 2 commits and net diff is 89 files changed (388 insertions,282 deletions).pkg/lightning/backend/localtopkg/ingestor/ingestctrland rewrite all old path references.local/local_testtoingestctrl/ingestctrl_test, and update all dependent code fromlocal.*toingestctrl.*.Scope of updates:
pkg/ingestor/ingestctrl.github.com/pingcap/tidb/pkg/ingestor/ingestctrl.GO_FAILPOINTSpaths.lightning,pkg/ddl,pkg/executor/importer,pkg/dxf/importinto,pkg/domain,pkg/server/handler, and related tests for the package identifier rename.Makefilemockgen source path andbuild/nogo_config.jsonmoved-file entries.Check List
Tests
Validation commands run:
./tools/check/failpoint-go-test.sh pkg/ingestor/ingestctrl -run TestGzipCompressor -count=1go test -run TestNonExistent -tags=intest,deadlock ./lightning/pkg/server ./lightning/pkg/importer ./pkg/ddl/ingest ./pkg/domain ./pkg/dxf/importinto ./pkg/executor/importer ./pkg/server/handler/tikvhandlergo test -run TestNonExistent -tags=intest,deadlock ./pkg/ddl ./tests/realtikvtest/addindextest/... ./tests/realtikvtest/addindextest1/... ./tests/realtikvtest/addindextest2/... ./tests/realtikvtest/addindextest3/... ./tests/realtikvtest/importintotest/... ./tests/realtikvtest/importintotest2/... ./tests/realtikvtest/importintotest4/...make lintSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit