Repository navigation
importinto, config: use synchronous prepare and add default max import size for Starter - #70511
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan includes up to 10 reviews per rolling hour; 9 remain after this review. 📝 WalkthroughWalkthroughChangesStarter import controls
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR changes Starter import preparation and applies a 25 GiB default size limit while preserving explicit settings; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant ConfigLoader
participant DeployMode
participant ShouldUseAsyncPrepare
participant ImportTask
ConfigLoader->>DeployMode: Load Starter or premium mode
DeployMode->>ShouldUseAsyncPrepare: Provide deployment mode
ShouldUseAsyncPrepare->>ImportTask: Select synchronous prepare for Starter
ShouldUseAsyncPrepare->>ImportTask: Select asynchronous prepare only for eligible modes
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
@zeminzhou: adding LGTM is restricted to approvers and reviewers in OWNERS files. 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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #70511 +/- ##
================================================
- Coverage 76.3256% 73.3161% -3.0096%
================================================
Files 2041 2083 +42
Lines 558170 585473 +27303
================================================
+ Hits 426027 429246 +3219
- Misses 131243 155608 +24365
+ Partials 900 619 -281
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
🔍 Starting code review for this PR... |
|
/retest |
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: 7
- Inline comments: 7
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
⚠️ [Major] (2)
- New 25GiB Starter default is inert for existing configs that carry the previously shipped
max-import-data-size = "0B"(pkg/config/config.go:1637, pkg/config/config.toml.nextgen.example:523) - Starter synchronous prepare is not actually bounded by max-import-data-size (pkg/dxf/importinto/job.go:78, pkg/executor/import_into.go:112, pkg/executor/importer/precheck.go:117, pkg/config/config_test.go:1454)
🟡 [Minor] (4)
- Starter import-size cap flips per-process during rolling restart with no operator-visible signal (pkg/config/config.go:1638, pkg/executor/importer/precheck.go:121)
- ShouldUseAsyncPrepare doc comment implies a size-dependent rationale the code doesn't apply (pkg/dxf/importinto/job.go:69, pkg/dxf/importinto/job_testkit_test.go:92)
- Starter sync path walks the data source twice per import (pkg/dxf/importinto/job.go:71, pkg/dxf/importinto/planner.go:391, pkg/dxf/importinto/scheduler.go:339)
TestShouldUseAsyncPreparetable varies an input the function never reads (pkg/dxf/importinto/job_testkit_test.go:92, pkg/dxf/importinto/job.go:78)
ℹ️ [Info] (1)
maxImportDataSizecolumn inTestShouldUseAsyncPreparenever affects the result (pkg/dxf/importinto/job_testkit_test.go:97, pkg/dxf/importinto/job.go:78)
yudongusa
left a comment
There was a problem hiding this comment.
This may need a document update PR
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: GMHDBJD, joechenrh, yudongusa, zeminzhou 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 |
What problem does this PR solve?
Issue Number: ref #67765
Problem Summary:
follow up of #68924
Starter imports are expected to be relatively small, and their maximum size is controlled by
starter-params.max-import-data-size. Using DXF asynchronous prepare adds scheduling overhead and delays validation feedback for these imports. Starter also previously inherited the unlimitedmax-import-data-sizedefault used by non-Starter deployment modes.What changed and how does it work?
ShouldUseAsyncPreparereturn false for every Starter deployment, so Starter prepares synchronously while other NextGen global-sort deployments continue to use asynchronous prepare.starter-params.max-import-data-sizeto25 * units.GiBwhen the deployment mode is Starter and the option is omitted.0Bfor unlimited imports, and keep the default at0Bfor non-Starter deployment modes.Check List
Tests
The following targeted tests and Ready-profile lint check passed locally:
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
IMPORT INTOsource data.0B.Bug Fixes