ttl: introduce starter ttl external 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
|
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:
📝 WalkthroughWalkthroughTiDB now propagates an external workload manager through startup, session and domain initialization, DDL TTL-table synchronization, TTL-job sysvar handling, and TTL worker ownership and recycling. ChangesExternal workload manager wiring
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TiDBServer
participant Session
participant Domain
participant DDL
participant TTLWorker
TiDBServer->>Session: Bootstrap with external workload manager
Session->>Domain: Configure external workload manager
Domain->>DDL: Initialize DDL with manager
DDL->>DDL: Register or delete TTL table metadata
Domain->>TTLWorker: Start worker for TTL task-worker role
TTLWorker->>TTLWorker: Recycle completed external TTL task
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69672 +/- ##
================================================
- Coverage 76.3321% 75.0885% -1.2436%
================================================
Files 2041 2107 +66
Lines 558864 587285 +28421
================================================
+ Hits 426593 440984 +14391
- Misses 131371 143990 +12619
- Partials 900 2311 +1411
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
|
/retest-required |
ystaticy
changed the title
ti-chi-bot
Bot
removed
do-not-merge/needs-tests-checked
do-not-merge/needs-linked-issue
labels
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
ti-chi-bot
Bot
added
size/XXL
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/session/session.go (1)
4391-4392: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the exported bootstrap API.
Add a Go doc comment beginning with
BootstrapSessionWithExternalWorkloadManagerthat explains its external-workload-manager behavior. As per coding guidelines, “Keep exported-symbol doc comments, and prefer semantic constraints over name restatement.”🤖 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 4391 - 4392, Add a Go doc comment immediately before BootstrapSessionWithExternalWorkloadManager that begins with the function name and describes that it bootstraps a session using the supplied external workload manager, emphasizing its behavioral contract rather than merely restating the name.Source: Coding guidelines
🤖 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/session/session_nextgen_test.go`:
- Around line 94-97: Update the test to invoke createSessionWithDomainOptions
directly instead of domap.getWithEtcdClient, passing the domainCreateOptions
containing mgr. Keep the existing no-error assertion and verify the created
session’s domain uses the same ExternalWorkloadManager instance.
---
Nitpick comments:
In `@pkg/session/session.go`:
- Around line 4391-4392: Add a Go doc comment immediately before
BootstrapSessionWithExternalWorkloadManager that begins with the function name
and describes that it bootstraps a session using the supplied external workload
manager, emphasizing its behavioral contract rather than merely restating the
name.
🪄 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 Plus
Run ID: f4095a2a-e3bd-4806-ab2e-991d1927bd4b
📒 Files selected for processing (7)
pkg/ddl/create_table.gopkg/ddl/ttl.gopkg/ddl/ttl_test.gopkg/session/session.gopkg/session/session_nextgen_test.gopkg/sessionctx/variable/sysvar.gopkg/sessionctx/variable/sysvar_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/ddl/ttl.go
- pkg/sessionctx/variable/sysvar.go
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
| } | ||
| return manager.RegisterTTLTask(ctx, tblInfo.ID, vardef.EnableTTLJob.Load()) | ||
| } | ||
|
|
There was a problem hiding this comment.
[P1] Reconcile failed TTL metadata synchronization
These RegisterTTLTask and DeleteTTLTableInfo calls are currently one-shot notifications: when the controller RPC fails, the error is only logged and the DDL still completes. Since the local TiDB metadata has already been committed, a transient controller outage can permanently leave the two sides inconsistent. For example, a successfully created TTL table may never be scheduled by the external worker if its registration is lost, while a failed delete may leave stale metadata for a dropped or disabled table.
Could we add a lightweight reconciliation mechanism instead of relying solely on these best-effort notifications? A simple option would be to periodically, and once during startup or controller recovery, enumerate the current enabled TTL tables and send an idempotent full snapshot to the controller so missing registrations are restored and stale entries are removed. If a full-snapshot API is not feasible, the failed register/delete operations should at least be persisted as pending sync records and retried after restart. The existing DDL notifications can remain as the fast path, while reconciliation provides recovery when an individual RPC fails.
There was a problem hiding this comment.
The current PR carries over an existing best-effort synchronization limitation from tidb-cse, and its create path makes the failure less visible by logging the error while still completing the DDL. A reconciliation mechanism would make the new external-workload TTL path self-healing.
There was a problem hiding this comment.
Thanks, good catch.
I changed this so RegisterTTLTask / DeleteTTLTableInfo are no longer best-effort for TTL DDL. If either call fails, the DDL now returns the error instead of succeeding silently.
For TRUNCATE on TTL tables, I also added compensation: if deleting the old registration succeeds but registering the new table fails, we try to restore the old table registration. This does not add full
reconciliation yet, but it removes the silent inconsistency path in this PR.
Signed-off-by: ystaticy <y_static_y@sina.com>
|
/retest-required |
| } | ||
| if jobCtx.oldDDLCtx != nil { | ||
| if oldTblInfo.TTLInfo != nil { | ||
| if err := jobCtx.oldDDLCtx.deleteTTLTableFromExternalWorkload(jobCtx.ctx, oldTblInfo.ID); err != nil { |
There was a problem hiding this comment.
[P1] Keep external TTL state consistent with the local truncate
This updates the external controller before the local truncate has completed. If DeleteTTLTableInfo(oldID) and RegisterTTLTask(newID) both succeed, but any later local operation (for example DropTableOrView, CreateTableOrView, or updateSchemaVersion) fails, the DDL job rolls back and TiDB still exposes the old table ID while the controller only knows about the new ID. The compensation below only covers RegisterTTLTask(newID) failing; it does not cover failures after both external calls succeed. The old table can then lose TTL scheduling, while the controller retains a registration for a table ID that does not exist locally. Could we move the external updates after the local DDL is durably committed, or add compensation for every subsequent local failure (including removing the new registration and restoring the old one)? The same ordering issue also applies to the DROP path around table.go:86-94.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ChangRui-Ryan 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 |
|
@ChangRui-Ryan: 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. |
Signed-off-by: ystaticy <y_static_y@sina.com>
|
/retest |
Signed-off-by: ystaticy <y_static_y@sina.com>
2 participants
What problem does this PR solve?
Issue Number: close #69962
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
TiDBTTLJobEnableupdates now propagate to the external controller (master-only).