ttl: introduce starter ttl worker#69672
Conversation
Signed-off-by: ystaticy <y_static_y@sina.com>
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
do-not-merge/needs-tests-checked
release-note-none
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
ti-chi-bot
Bot
added
the
size/XL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR threads an ChangesExternal workload manager wiring
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: context deadline exceeded" 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69672 +/- ##
================================================
- Coverage 76.3268% 75.9537% -0.3732%
================================================
Files 2041 2077 +36
Lines 560581 578846 +18265
================================================
+ Hits 427874 439655 +11781
- Misses 131806 137179 +5373
- Partials 901 2012 +1111
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/session/session.go (1)
4360-4361: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAttach the external workload manager before bootstrap creates the domain
pkg/session/session.go:4360-4396
runInBootstrapSession(store, ver)creates and caches the domain first; the laterdomap.getWithEtcdClient(..., domainCreateOptions{extWorkloadMgr: extWorkloadMgr})call then returns the cached domain without applyingSetExternalWorkloadManager. That leaves bootstrap/upgrade runs (ver < currentBootstrapVersion) without the external workload manager attached. Move the wiring ahead of bootstrap, or pass the manager through the bootstrap session path.🤖 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 `@pkg/session/session.go` around lines 4360 - 4361, Attach the external workload manager before bootstrap initializes the domain, because runInBootstrapSession(store, ver) caches the domain too early and the later domap.getWithEtcdClient(..., domainCreateOptions{extWorkloadMgr: extWorkloadMgr}) path won’t reapply SetExternalWorkloadManager. Update the ver < currentBootstrapVersion flow in session/session.go so extWorkloadMgr is wired into the bootstrap session path itself, or ensure it is passed into the domain creation path before the domain is cached.pkg/sessionctx/variable/sysvar.go (1)
3218-3227: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winLocal flag is mutated before the external propagation can fail, causing inconsistent state on error.
vardef.EnableTTLJob.Store(enable)happens unconditionally, thenUpdateExternalWorkloadTTLJobEnableis called and its error is returned. If the external call fails,SET GLOBAL tidb_ttl_job_enable=...reports an error to the user, butvardef.EnableTTLJob(consulted elsewhere, e.g. TTL table registration inpkg/ddl/ttl.go) has already changed — the client sees a failure while the internal state actually flipped.🛠️ Suggested fix: only commit local state after the external call succeeds
{Scope: vardef.ScopeGlobal, Name: vardef.TiDBTTLJobEnable, Value: BoolToOnOff(vardef.DefTiDBTTLJobEnable), Type: vardef.TypeBool, SetGlobal: func(ctx context.Context, vars *SessionVars, s string) error { enable := TiDBOptOn(s) - vardef.EnableTTLJob.Store(enable) - if UpdateExternalWorkloadTTLJobEnable != nil { - return UpdateExternalWorkloadTTLJobEnable(ctx, enable) - } + if UpdateExternalWorkloadTTLJobEnable != nil { + if err := UpdateExternalWorkloadTTLJobEnable(ctx, enable); err != nil { + return err + } + } + vardef.EnableTTLJob.Store(enable) return nil }, GetGlobal: func(ctx context.Context, vars *SessionVars) (string, error) {🤖 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 `@pkg/sessionctx/variable/sysvar.go` around lines 3218 - 3227, The TiDBTTLJobEnable SetGlobal handler mutates vardef.EnableTTLJob before UpdateExternalWorkloadTTLJobEnable can fail, leaving local state inconsistent with the returned error. In the SetGlobal closure for TiDBTTLJobEnable, move the vardef.EnableTTLJob.Store(enable) update so it only happens after UpdateExternalWorkloadTTLJobEnable(ctx, enable) succeeds, and preserve the current error return path if the external propagation fails. Keep the change localized to the TiDBTTLJobEnable sysvar registration and use the existing vardef.EnableTTLJob and UpdateExternalWorkloadTTLJobEnable symbols to ensure the in-memory flag only reflects a successful global update.pkg/ddl/ttl.go (1)
79-104: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate table-level TTL disable to the external controller
ALTER TABLE ... TTL_ENABLE='OFF'clears the local TTL flag, but this path only callsregisterTTLTableToExternalWorkload, which returns immediately for disabled TTL. The previous external record is never removed, so the controller can keep treating the table as enabled.🤖 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 `@pkg/ddl/ttl.go` around lines 79 - 104, The TTL update path in ttl.go only re-registers the table with the external workload controller, so disabling TTL via ALTER TABLE ... TTL_ENABLE='OFF' leaves stale state behind. In the TTL update flow around updateVersionAndTableInfo and jobCtx.oldDDLCtx.registerTTLTableToExternalWorkload, add an explicit branch for tblInfo.TTLInfo.Enable == false that removes/unregisters the table from the external controller before returning, while keeping the existing register path for enabled TTL.
🧹 Nitpick comments (2)
pkg/domain/domain.go (1)
2891-2907: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a comment explaining the role-gating semantics.
shouldStartTTLJobManagerencodes non-obvious behavior: when external workload is enabled, the TTL job manager only starts for theTTLTaskWorkerrole and is skipped forMaster/other roles, whereas it always starts when external workload is disabled. This inversion (master normally runs it, but not under Starter mode) is easy to misread; a short comment would help future maintainers.📝 Suggested comment
+// shouldStartTTLJobManager reports whether this instance should run the local +// TTL job manager. When external workload coordination is disabled, every +// instance runs it as before. When enabled, only the TTL task worker role +// runs it locally; other roles (e.g. master) delegate TTL job execution to +// the dedicated TTL task worker instances. func (do *Domain) shouldStartTTLJobManager() bool {As per coding guidelines, "Comments SHOULD explain non-obvious intent, constraints, invariants... SHOULD NOT restate what the code already makes clear."
🤖 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 `@pkg/domain/domain.go` around lines 2891 - 2907, Add a brief comment near shouldStartTTLJobManager that explains the role-gating intent: TTL job manager starts unconditionally when external workload is disabled, but when extworkload is enabled it only starts for the TTLTaskWorker role and is skipped for Master/other roles. Keep the comment focused on this non-obvious Starter mode inversion so future readers understand why StartTTLJobManager delegates to shouldStartTTLJobManager.Source: Coding guidelines
pkg/ddl/ttl.go (1)
109-135: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a bounded timeout for the external controller calls.
registerTTLTableToExternalWorkload/deleteTTLTableFromExternalWorkloadforwardjobCtx.ctxdirectly toRegisterTTLTask/DeleteTTLTableInfowith no deadline. If the external controller is slow/unresponsive, these calls can stall the DDL job worker for an unbounded time (this matters even more givenonCreateTabletreats a failure as fatal — see companion comment in create_table.go).🤖 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 `@pkg/ddl/ttl.go` around lines 109 - 135, Both registerTTLTableToExternalWorkload and deleteTTLTableFromExternalWorkload pass the caller context straight into external controller calls with no deadline, which can block DDL work indefinitely. Wrap the manager.RegisterTTLTask and manager.DeleteTTLTableInfo calls with a bounded timeout context inside these helpers, and ensure the timeout is canceled promptly after the call. Keep the change localized to externalWorkloadMaster usage so the behavior is applied consistently for both TTL registration and deletion.
🤖 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 `@pkg/ddl/create_table.go`:
- Around line 252-255: The CREATE TABLE flow currently treats
registerTTLTableToExternalWorkload as fatal, which can abort the DDL job after
the table has already been created and versioned. Update the create table path
in create_table.go so the external workload registration failure is handled
non-fatally, matching the behavior in onTTLInfoChange in ttl.go: log a warning
with the error and continue so job.FinishTableJob can still run. Keep the fix
localized around registerTTLTableToExternalWorkload and preserve the existing
DDL transaction flow.
In `@pkg/domain/domain.go`:
- Around line 235-238: The `// only used for nextgen` comment is now misleading
because it also appears to describe `extWorkloadMgr`, which belongs to a
different feature path. Update the comment near
`crossKSSessMgr`/`crossKSSessFactoryGetter` in `domain.go` so it applies only to
those nextgen-specific fields, and move or add a separate comment for
`extWorkloadMgr` that reflects its Starter deploy/external-workload purpose.
---
Outside diff comments:
In `@pkg/ddl/ttl.go`:
- Around line 79-104: The TTL update path in ttl.go only re-registers the table
with the external workload controller, so disabling TTL via ALTER TABLE ...
TTL_ENABLE='OFF' leaves stale state behind. In the TTL update flow around
updateVersionAndTableInfo and
jobCtx.oldDDLCtx.registerTTLTableToExternalWorkload, add an explicit branch for
tblInfo.TTLInfo.Enable == false that removes/unregisters the table from the
external controller before returning, while keeping the existing register path
for enabled TTL.
In `@pkg/session/session.go`:
- Around line 4360-4361: Attach the external workload manager before bootstrap
initializes the domain, because runInBootstrapSession(store, ver) caches the
domain too early and the later domap.getWithEtcdClient(...,
domainCreateOptions{extWorkloadMgr: extWorkloadMgr}) path won’t reapply
SetExternalWorkloadManager. Update the ver < currentBootstrapVersion flow in
session/session.go so extWorkloadMgr is wired into the bootstrap session path
itself, or ensure it is passed into the domain creation path before the domain
is cached.
In `@pkg/sessionctx/variable/sysvar.go`:
- Around line 3218-3227: The TiDBTTLJobEnable SetGlobal handler mutates
vardef.EnableTTLJob before UpdateExternalWorkloadTTLJobEnable can fail, leaving
local state inconsistent with the returned error. In the SetGlobal closure for
TiDBTTLJobEnable, move the vardef.EnableTTLJob.Store(enable) update so it only
happens after UpdateExternalWorkloadTTLJobEnable(ctx, enable) succeeds, and
preserve the current error return path if the external propagation fails. Keep
the change localized to the TiDBTTLJobEnable sysvar registration and use the
existing vardef.EnableTTLJob and UpdateExternalWorkloadTTLJobEnable symbols to
ensure the in-memory flag only reflects a successful global update.
---
Nitpick comments:
In `@pkg/ddl/ttl.go`:
- Around line 109-135: Both registerTTLTableToExternalWorkload and
deleteTTLTableFromExternalWorkload pass the caller context straight into
external controller calls with no deadline, which can block DDL work
indefinitely. Wrap the manager.RegisterTTLTask and manager.DeleteTTLTableInfo
calls with a bounded timeout context inside these helpers, and ensure the
timeout is canceled promptly after the call. Keep the change localized to
externalWorkloadMaster usage so the behavior is applied consistently for both
TTL registration and deletion.
In `@pkg/domain/domain.go`:
- Around line 2891-2907: Add a brief comment near shouldStartTTLJobManager that
explains the role-gating intent: TTL job manager starts unconditionally when
external workload is disabled, but when extworkload is enabled it only starts
for the TTLTaskWorker role and is skipped for Master/other roles. Keep the
comment focused on this non-obvious Starter mode inversion so future readers
understand why StartTTLJobManager delegates to shouldStartTTLJobManager.
🪄 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: 054f938c-9624-4889-9304-62705ac19645
📒 Files selected for processing (21)
cmd/tidb-server/main.gopkg/ddl/BUILD.bazelpkg/ddl/create_table.gopkg/ddl/ddl.gopkg/ddl/options.gopkg/ddl/ttl.gopkg/ddl/ttl_test.gopkg/domain/BUILD.bazelpkg/domain/domain.gopkg/domain/domain_sysvars.gopkg/domain/domain_test.gopkg/session/BUILD.bazelpkg/session/bootstrap_test.gopkg/session/session.gopkg/session/session_test.gopkg/session/tidb.gopkg/sessionctx/variable/sysvar.gopkg/sessionctx/variable/tidb_vars.gopkg/ttl/ttlworker/BUILD.bazelpkg/ttl/ttlworker/job_manager.gopkg/ttl/ttlworker/job_manager_test.go
| if err := w.registerTTLTableToExternalWorkload(jobCtx.ctx, tbInfo); err != nil { | ||
| return ver, errors.Trace(err) | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
External workload registration failure aborts CREATE TABLE — inconsistent with ttl.go and risks cascading DDL outages.
Unlike onTTLInfoChange in pkg/ddl/ttl.go (which logs a warning and continues on registration failure), this path returns the error and prevents job.FinishTableJob from being called, effectively failing/retrying the whole CREATE TABLE DDL job whenever the external workload controller is unavailable or slow. Since table creation, schema version bump, and event notification have already happened in this transaction, an external RPC failure here couples DDL availability for TTL tables to an external dependency (with no external-call hazard mitigation such as short timeout/circuit breaker visible).
🐛 Suggested fix: make external registration failure non-fatal, consistent with ttl.go
- if err := w.registerTTLTableToExternalWorkload(jobCtx.ctx, tbInfo); err != nil {
- return ver, errors.Trace(err)
- }
+ if err := w.registerTTLTableToExternalWorkload(jobCtx.ctx, tbInfo); err != nil {
+ logutil.DDLLogger().Warn("failed to register TTL table to external workload controller",
+ zap.Int64("tableID", tbInfo.ID),
+ zap.Error(err))
+ }📝 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.
| if err := w.registerTTLTableToExternalWorkload(jobCtx.ctx, tbInfo); err != nil { | |
| return ver, errors.Trace(err) | |
| } | |
| if err := w.registerTTLTableToExternalWorkload(jobCtx.ctx, tbInfo); err != nil { | |
| logutil.DDLLogger().Warn("failed to register TTL table to external workload controller", | |
| zap.Int64("tableID", tbInfo.ID), | |
| zap.Error(err)) | |
| } |
🤖 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 `@pkg/ddl/create_table.go` around lines 252 - 255, The CREATE TABLE flow
currently treats registerTTLTableToExternalWorkload as fatal, which can abort
the DDL job after the table has already been created and versioned. Update the
create table path in create_table.go so the external workload registration
failure is handled non-fatally, matching the behavior in onTTLInfoChange in
ttl.go: log a warning with the error and continue so job.FinishTableJob can
still run. Keep the fix localized around registerTTLTableToExternalWorkload and
preserve the existing DDL transaction flow.
| // only used for nextgen | ||
| crossKSSessMgr *crossks.Manager | ||
| crossKSSessFactoryGetter func(string, validatorapi.Validator) pools.Factory | ||
| extWorkloadMgr extworkload.Manager |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Misleading comment scope for new field.
extWorkloadMgr is appended directly under the // only used for nextgen comment, which was written to describe crossKSSessMgr/crossKSSessFactoryGetter. Per the extworkload package doc, the manager is created "in Starter deploy mode when [external-workload] is enabled" — a different concept from "nextgen" kernel type. Leaving it grouped under this comment will mislead future readers into thinking extWorkloadMgr is nextgen-only.
✏️ Proposed fix
// only used for nextgen
crossKSSessMgr *crossks.Manager
crossKSSessFactoryGetter func(string, validatorapi.Validator) pools.Factory
- extWorkloadMgr extworkload.Manager
+
+ // extWorkloadMgr coordinates background workloads (TTL, GC, auto-analyze) with
+ // an external workload controller when running in Starter deploy mode.
+ extWorkloadMgr extworkload.Manager
}As per coding guidelines, "Comments SHOULD explain non-obvious intent... SHOULD NOT restate what the code already makes clear" — here the existing comment inaccurately extends to unrelated code.
📝 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.
| // only used for nextgen | |
| crossKSSessMgr *crossks.Manager | |
| crossKSSessFactoryGetter func(string, validatorapi.Validator) pools.Factory | |
| extWorkloadMgr extworkload.Manager | |
| // only used for nextgen | |
| crossKSSessMgr *crossks.Manager | |
| crossKSSessFactoryGetter func(string, validatorapi.Validator) pools.Factory | |
| // extWorkloadMgr coordinates background workloads (TTL, GC, auto-analyze) with | |
| // an external workload controller when running in Starter deploy mode. | |
| extWorkloadMgr extworkload.Manager |
🤖 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 `@pkg/domain/domain.go` around lines 235 - 238, The `// only used for nextgen`
comment is now misleading because it also appears to describe `extWorkloadMgr`,
which belongs to a different feature path. Update the comment near
`crossKSSessMgr`/`crossKSSessFactoryGetter` in `domain.go` so it applies only to
those nextgen-specific fields, and move or add a separate comment for
`extWorkloadMgr` that reflects its Starter deploy/external-workload purpose.
Source: Coding guidelines
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. Notice: To remove the For example:
📖 For more info, you can check the "Contribute Code" section in the development guide. |
|
/retest-required |
|
@ystaticy: 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. |
1 participant
What problem does this PR solve?
Issue Number: close #xxx
Problem Summary:
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit