Release 6.5.6 hotfix 20260409 - #67725
Conversation
Signed-off-by: HunDunDM <hundundm@gmail.com>
Signed-off-by: HunDunDM <hundundm@gmail.com>
Signed-off-by: HunDunDM <hundundm@gmail.com>
Signed-off-by: HunDunDM <hundundm@gmail.com>
ti-chi-bot
Bot
added
do-not-merge/invalid-title
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 |
|
Hi @HunDunDM. 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:
📝 WalkthroughWalkthroughAdds PD force-merge range computation/dispatch for GC, a domain background scanner with checkpointing and reset API, delete-range integration, running-jobs tracking changes with an owner-retire hook, a sysvar feature flag, and many accompanying tests and build updates. Changes
Sequence Diagram(s)sequenceDiagram
participant DeleteRange as DeleteRangeEmulator
participant DDL as DDL
participant InfoSync as InfoSync
participant PD as PD
DeleteRange->>DDL: notify task complete (JobID, StartKey, EndKey)
DDL->>DDL: load history Job by JobID
DDL->>DDL: GetForceMergeRangesForGCDeleteRange(job, dr, cache)
DDL-->>DeleteRange: []ForceMergeKeyRange (physical [+ logical if needed])
DeleteRange->>InfoSync: AddForceMergeRanges(ctx, ranges)
loop per-batch (<=2048)
InfoSync->>PD: POST /regions/force-merge (hex-encoded batch body)
PD-->>InfoSync: response / timeout / error
InfoSync->>InfoSync: sleep between batches (respect ctx)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
owner/mock.go (1)
64-66:⚠️ Potential issue | 🟠 Major
SetRetireOwnerHookis a no-op becauseRetireOwnernever callsretireHook.The new hook is wired but never executed, so retire-time side effects won’t run in
mockManager.💡 Suggested fix
func (m *mockManager) RetireOwner() { + if m.retireHook != nil { + m.retireHook() + } atomic.StoreInt32(&m.owner, 0) }Also applies to: 105-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@owner/mock.go` around lines 64 - 66, The mockManager's SetRetireOwnerHook is never exercised because RetireOwner doesn't call the hook; update func (m *mockManager) RetireOwner() to invoke m.retireHook() (guarded by a nil check) after atomic.StoreInt32(&m.owner, 0); also apply the same change to the other retire path referenced in the file (the second method that sets m.owner to 0 around the other retire implementation) so both retire flows call m.retireHook() when present.ddl/ddl_running_jobs.go (1)
68-132:⚠️ Potential issue | 🔴 Critical
unfinishedSchemanow blocks the same unfinished job from running again.
remove()keeps the schema entry for any non-terminal job, butcheckRunnable()blindly rejects any job whose schema/table is present there. After the first processing round, the job leavesprocessingIDsyet still matches its ownunfinishedSchemaentry, so it can no longer be dispatched. With the current set-only structure, there's no way to distinguish “this job” from “another conflicting job.”This needs per-schema job IDs/refcounts, or an explicit self-ignore path, before the new unfinished-state split is safe.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddl/ddl_running_jobs.go` around lines 68 - 132, The unfinishedSchema currently blocks a job from being rescheduled because it only records schema/table presence; change it to track per-schema job IDs (e.g. unfinishedSchema: map[string]map[string]map[JobID]struct{} or add a per-schema refcount/map) and update runningJobs.remove and wherever unfinishedSchema is written to add/remove the job.ID instead of just toggling presence; then modify runningJobs.checkRunnable to ignore unfinishedSchema entries that only contain the same job.ID (i.e., allow self-ignore) and only treat a schema/table as conflicting when it contains any job ID != job.ID. Ensure updateInternalRunningJobIDs and processingIDs logic remains consistent with the new structure.store/gcworker/gc_worker.go (1)
723-739:⚠️ Potential issue | 🟠 MajorDon't complete the delete-range task before the force-merge report is durable.
CompleteDeleteRangeruns beforedoGCForceMergeRanges, but a force-merge failure is only logged. That means a transientAddForceMergeRangeserror permanently drops the report, because this task will no longer be loaded fromgc_delete_rangeon the next GC round. The opposite edge also happens today: whenCompleteDeleteRangefails, you still report force-merge ranges and can emit duplicates on retry.Please make the report path retryable with the task state instead of best-effort after completion.
Also applies to: 766-783
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@store/gcworker/gc_worker.go` around lines 723 - 739, The code currently calls util.CompleteDeleteRange before doGCForceMergeRanges, which discards a transient AddForceMergeRanges failure; swap the order and make the force-merge report durable/required before completing the delete-range task: call w.doGCForceMergeRanges(ctx, se, r, gcForceMergeTableCache) first and if it returns an error, return that error (do not call util.CompleteDeleteRange) so the task remains retriable; only after doGCForceMergeRanges succeeds should you call util.CompleteDeleteRange and handle its error separately; ensure AddForceMergeRanges/reporting path is idempotent or persisted so retries won’t double-report (use r.JobID/r.ElementID as idempotency keys) and apply the same change for the other affected block (the 766-783 region).ddl/delete_range.go (1)
251-257:⚠️ Potential issue | 🟠 MajorDon't delete the retry source before sending the force-merge report.
Line 251 completes the
gc_delete_rangetask before Lines 255-257 attempt the PD report. If that report fails transiently, orctxis canceled during shutdown, the error is only logged and there is no task left to retry from, so the force-merge request can be dropped permanently. Please either make reporting happen before completion, or persist/retry it independently of the delete-range row.Also applies to: 274-287
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ddl/delete_range.go` around lines 251 - 257, The code currently calls util.CompleteDeleteRange(sctx, r) before attempting dr.reportForceMergeRanges(...), which can drop the retry source if reporting fails; change the order so that dr.reportForceMergeRanges(ctx, sctx, r, gcForceMergeTableCache) is attempted (and retried/persisted on transient failure) before calling util.CompleteDeleteRange(sctx, r), or alternatively make reportForceMergeRanges persist its own retry record independent of the gc_delete_range row so failures do not remove the ability to retry; update the blocks around util.CompleteDeleteRange and dr.reportForceMergeRanges (and similarly the other occurrence that mirrors lines 274-287) to ensure reporting is durable or executed prior to completing the delete-range row.
🧹 Nitpick comments (1)
owner/manager.go (1)
62-63: Fix the retire-hook contract comment.The new interface comment says the hook runs after retiring the owner, but
RetireOwner()actually invokes it before clearingm.elec. That ordering matters here, so the public comment should match the real contract.As per coding guidelines, "Comments SHOULD explain non-obvious intent, constraints, invariants, concurrency guarantees, SQL/compatibility contracts, or important performance trade-offs, and SHOULD NOT restate what the code already makes clear."
Also applies to: 195-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@owner/manager.go` around lines 62 - 63, Update the comment for SetRetireOwnerHook to reflect the actual contract: the hook is invoked by RetireOwner() before the manager clears m.elec (i.e., prior to removing owner state), not after; change both occurrences (the interface comment near SetRetireOwnerHook and the similar comment at the other location) to state that the hook runs before clearing owner/election state so the documented ordering matches the behavior in RetireOwner and preserves the intended concurrency/invariant reasoning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ddl/ddl.go`:
- Around line 757-759: The retire-owner hook currently swaps the shared pointer
by calling d.ownerManager.SetRetireOwnerHook(func() { d.runningJobs =
newRunningJobs() }), which is unsafe because concurrent goroutines like
delivery2worker will keep referencing the old instance; instead, add a
Clear/Reset method on the runningJobs type (or reuse an existing lock-protected
method) that zeroes its internals under its own mutex, then change the hook to
call d.runningJobs.Clear() (or Reset()) rather than replacing the pointer so the
runningJobs pointer remains stable while its contents are cleared safely under
lock.
In `@domain/infosync/info.go`:
- Around line 392-399: doRequest currently captures and reuses the same
io.Reader across retries, which causes later attempts to read from a partially
consumed reader; fix by reading the full body into a byte slice if body != nil
and call doRequestWithBodyBytes(ctx, apiName, addrs, route, method, bodyBytes)
so each retry gets a fresh reader (or otherwise construct a new bytes.Reader per
attempt); update the doRequest function to convert the incoming body to bytes
and delegate to doRequestWithBodyBytes instead of creating a closure that
returns the same io.Reader, referencing doRequest, doRequestWithBodyFactory and
doRequestWithBodyBytes to locate the change.
In `@domain/pkdb_force_merge.go`:
- Around line 104-121: The helpers loadOrInitMergeEmptyRegionsMinTableID and the
similar loadMergeEmptyRegionsMaxTableID replace the caller context with
context.Background(), blocking cancellation; change their signatures to accept a
context.Context parameter and use that ctx when calling
kv.WithInternalSourceType (i.e., replace context.Background() with the passed
ctx inside kv.RunInNewTxn), and update the caller doMergeEmptyRegions to pass
its ctx through so the checkpoint KV operations become cancellation-aware.
Ensure all call sites are updated to provide the context and adjust any error
handling signatures accordingly.
In `@executor/update.go`:
- Around line 85-93: The current logic sets skipMultipleChangesOnSameRow=false
for all non-clustered PartitionedTable instances, which globally disables dedup
and causes repeated updates; instead, implement a partition-aware dedup keyed by
(physical partition ID + handle). Change the branch that checks
tbl.(table.PartitionedTable) / tbl.Meta().HasClusteredIndex() to stop flipping a
global flag and instead ensure the update executor (where e.changed is cleared
for repeated handles) consults a new per-table map (e.g.,
seenHandlesByPartition) keyed by the partition identity plus the row handle;
mark entries when a physical (partitionID, handle) pair is updated so subsequent
matches for the same physical row skip setting e.changed=false only if the same
physical key was seen, leaving cross-partition duplicates unaffected. Ensure the
map is created/cleared per statement and referenced where e.changed is evaluated
for repeated handles.
In `@sessionctx/variable/pkdb_force_merge_test.go`:
- Around line 24-58: The unit test only covers the in-process hook; add a SQL
integration test named e.g. TestSetTiDBEnableDropTableForceMerge_SQL that uses
the SQL test harness (TestKit) to SET the variable and read it back via SQL to
cover the user-visible path: execute "SET GLOBAL
tidb_enable_drop_table_force_merge=ON" (and OFF), then verify with "SELECT
@@global.tidb_enable_drop_table_force_merge" and "SELECT
@@tidb_enable_drop_table_force_merge" (and session-level checks if appropriate),
and ensure cleanup/restore of the original value; reference the existing
TestSetTiDBEnableDropTableForceMerge, the sysvar TiDBEnableDropTableForceMerge
and the boolean flag EnableDropTableForceMerge when asserting effects.
In `@util/printer/printer.go`:
- Line 171: The compiler directive on the line containing the pseudo-symbol
buildVersion is malformed: remove the space between the comment slashes and the
directive so the link directive uses the exact "//go:linkname" prefix and links
buildVersion to runtime.buildVersion; update the line in printer.go where "//
go:linkname buildVersion runtime.buildVersion" appears to use the correct
"//go:linkname" form so the compiler recognizes the directive.
---
Outside diff comments:
In `@ddl/ddl_running_jobs.go`:
- Around line 68-132: The unfinishedSchema currently blocks a job from being
rescheduled because it only records schema/table presence; change it to track
per-schema job IDs (e.g. unfinishedSchema:
map[string]map[string]map[JobID]struct{} or add a per-schema refcount/map) and
update runningJobs.remove and wherever unfinishedSchema is written to add/remove
the job.ID instead of just toggling presence; then modify
runningJobs.checkRunnable to ignore unfinishedSchema entries that only contain
the same job.ID (i.e., allow self-ignore) and only treat a schema/table as
conflicting when it contains any job ID != job.ID. Ensure
updateInternalRunningJobIDs and processingIDs logic remains consistent with the
new structure.
In `@ddl/delete_range.go`:
- Around line 251-257: The code currently calls util.CompleteDeleteRange(sctx,
r) before attempting dr.reportForceMergeRanges(...), which can drop the retry
source if reporting fails; change the order so that
dr.reportForceMergeRanges(ctx, sctx, r, gcForceMergeTableCache) is attempted
(and retried/persisted on transient failure) before calling
util.CompleteDeleteRange(sctx, r), or alternatively make reportForceMergeRanges
persist its own retry record independent of the gc_delete_range row so failures
do not remove the ability to retry; update the blocks around
util.CompleteDeleteRange and dr.reportForceMergeRanges (and similarly the other
occurrence that mirrors lines 274-287) to ensure reporting is durable or
executed prior to completing the delete-range row.
In `@owner/mock.go`:
- Around line 64-66: The mockManager's SetRetireOwnerHook is never exercised
because RetireOwner doesn't call the hook; update func (m *mockManager)
RetireOwner() to invoke m.retireHook() (guarded by a nil check) after
atomic.StoreInt32(&m.owner, 0); also apply the same change to the other retire
path referenced in the file (the second method that sets m.owner to 0 around the
other retire implementation) so both retire flows call m.retireHook() when
present.
In `@store/gcworker/gc_worker.go`:
- Around line 723-739: The code currently calls util.CompleteDeleteRange before
doGCForceMergeRanges, which discards a transient AddForceMergeRanges failure;
swap the order and make the force-merge report durable/required before
completing the delete-range task: call w.doGCForceMergeRanges(ctx, se, r,
gcForceMergeTableCache) first and if it returns an error, return that error (do
not call util.CompleteDeleteRange) so the task remains retriable; only after
doGCForceMergeRanges succeeds should you call util.CompleteDeleteRange and
handle its error separately; ensure AddForceMergeRanges/reporting path is
idempotent or persisted so retries won’t double-report (use r.JobID/r.ElementID
as idempotency keys) and apply the same change for the other affected block (the
766-783 region).
---
Nitpick comments:
In `@owner/manager.go`:
- Around line 62-63: Update the comment for SetRetireOwnerHook to reflect the
actual contract: the hook is invoked by RetireOwner() before the manager clears
m.elec (i.e., prior to removing owner state), not after; change both occurrences
(the interface comment near SetRetireOwnerHook and the similar comment at the
other location) to state that the hook runs before clearing owner/election state
so the documented ordering matches the behavior in RetireOwner and preserves the
intended concurrency/invariant reasoning.
🪄 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: 4b5e1d72-0e91-4bc1-b0fc-a6dd715ef6cc
📒 Files selected for processing (32)
ddl/BUILD.bazelddl/db_partition_test.goddl/ddl.goddl/ddl_running_jobs.goddl/ddl_running_jobs_test.goddl/delete_range.goddl/pkdb_force_merge.goddl/pkdb_force_merge_test.godomain/BUILD.bazeldomain/domain.godomain/infosync/BUILD.bazeldomain/infosync/info.godomain/infosync/pkdb_force_merge.godomain/infosync/pkdb_force_merge_test.godomain/pkdb_force_merge.godomain/pkdb_force_merge_test.goexecutor/update.goinfoschema/builder.gometa/meta.gometa/meta_test.goowner/manager.goowner/mock.goserver/http_handler.goserver/http_handler_test.goserver/http_status.gosessionctx/variable/BUILD.bazelsessionctx/variable/pkdb_force_merge_test.gosessionctx/variable/session.gosessionctx/variable/sysvar.gosessionctx/variable/tidb_vars.gostore/gcworker/gc_worker.goutil/printer/printer.go
| d.ownerManager.SetRetireOwnerHook(func() { | ||
| d.runningJobs = newRunningJobs() | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all pointer-write/read sites for runningJobs and hook wiring.
rg -nP 'SetRetireOwnerHook|d\.runningJobs\s*=' -C2
rg -nP '\bd\.runningJobs\.' -C2Repository: pingcap/tidb
Length of output: 2976
🏁 Script executed:
# Find the runningJobs struct definition and its locking mechanism
fd -e go | xargs grep -l "type.*runningJobs" | head -5Repository: pingcap/tidb
Length of output: 79
🏁 Script executed:
# Get the full definition of runningJobs struct
rg -A 20 "type\s+runningJobs\s+struct" ddl/Repository: pingcap/tidb
Length of output: 1255
🏁 Script executed:
# Examine the RetireOwner implementation to understand hook invocation semantics
rg -B 5 -A 15 "func.*RetireOwner\(" owner/manager.goRepository: pingcap/tidb
Length of output: 610
🏁 Script executed:
# Check if d.runningJobs is assigned to local variables or captured in goroutines
rg -n "runningJobs\s*:=|=.*runningJobs|runningJobs\s*\)" ddl/ owner/Repository: pingcap/tidb
Length of output: 505
🏁 Script executed:
# Examine the add/remove method implementations to confirm they use the lock
rg -B 2 -A 10 "func \(j \*runningJobs\) (add|remove)" ddl/Repository: pingcap/tidb
Length of output: 1325
🏁 Script executed:
# Check if there are any direct field accesses on d.runningJobs or runningJobs (e.g., d.runningJobs.processingIDs)
rg "\.processingIDs|\.unfinishedIDs|\.unfinishedSchema" ddl/Repository: pingcap/tidb
Length of output: 1298
Avoid swapping d.runningJobs pointer inside retire-owner hook.
Lines 757–759 replace a shared pointer unsafely. Async goroutines spawned before the swap (e.g., delivery2worker at job_table.go:210–215) will reference the old instance, causing jobs to be added to one instance and removed from another.
Safer approach: keep pointer stable, clear internals under lock
d.ownerManager.SetRetireOwnerHook(func() {
- d.runningJobs = newRunningJobs()
+ d.runningJobs.Lock()
+ defer d.runningJobs.Unlock()
+ d.runningJobs.processingIDs = make(map[int64]struct{})
+ d.runningJobs.processingIDsStr = ""
+ d.runningJobs.unfinishedIDs = make(map[int64]struct{})
+ d.runningJobs.unfinishedSchema = make(map[string]map[string]struct{})
})📝 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.
| d.ownerManager.SetRetireOwnerHook(func() { | |
| d.runningJobs = newRunningJobs() | |
| }) | |
| d.ownerManager.SetRetireOwnerHook(func() { | |
| d.runningJobs.Lock() | |
| defer d.runningJobs.Unlock() | |
| d.runningJobs.processingIDs = make(map[int64]struct{}) | |
| d.runningJobs.processingIDsStr = "" | |
| d.runningJobs.unfinishedIDs = make(map[int64]struct{}) | |
| d.runningJobs.unfinishedSchema = make(map[string]map[string]struct{}) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ddl/ddl.go` around lines 757 - 759, The retire-owner hook currently swaps the
shared pointer by calling d.ownerManager.SetRetireOwnerHook(func() {
d.runningJobs = newRunningJobs() }), which is unsafe because concurrent
goroutines like delivery2worker will keep referencing the old instance; instead,
add a Clear/Reset method on the runningJobs type (or reuse an existing
lock-protected method) that zeroes its internals under its own mutex, then
change the hook to call d.runningJobs.Clear() (or Reset()) rather than replacing
the pointer so the runningJobs pointer remains stable while its contents are
cleared safely under lock.
| func doRequest(ctx context.Context, apiName string, addrs []string, route, method string, body io.Reader) ([]byte, error) { | ||
| var bodyFactory func() io.Reader | ||
| if body != nil { | ||
| bodyFactory = func() io.Reader { | ||
| return body | ||
| } | ||
| } | ||
| return doRequestWithBodyFactory(ctx, apiName, addrs, route, method, bodyFactory) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== All doRequest call sites in domain/infosync ==="
rg -n --type=go '\bdoRequest\s*\(' domain/infosync -C2
echo
echo "=== Likely non-nil body call sites (manual check required) ==="
# Shows call sites; verify the 6th argument is not nil and whether call should move to doRequestWithBodyBytes.
rg -n --type=go '\bdoRequest\s*\(' domain/infosync -C4 | rg -n -v 'nil'Repository: pingcap/tidb
Length of output: 19972
🏁 Script executed:
cat -n domain/infosync/info.go | sed -n '380,450p'Repository: pingcap/tidb
Length of output: 2991
doRequest reuses a consumed request body across address retries.
The closure captures the io.Reader and returns the same instance for each address. After the first attempt, the reader is partially consumed; subsequent retries use it from the wrong position. While all current call sites work around this by passing fresh bytes.NewBuffer() or bytes.NewReader() objects, the abstraction is fragile and violates io.Reader semantics.
Use doRequestWithBodyBytes instead, which correctly creates a new reader per attempt via the factory function.
💡 Suggested fix
func doRequest(ctx context.Context, apiName string, addrs []string, route, method string, body io.Reader) ([]byte, error) {
- var bodyFactory func() io.Reader
- if body != nil {
- bodyFactory = func() io.Reader {
- return body
- }
- }
- return doRequestWithBodyFactory(ctx, apiName, addrs, route, method, bodyFactory)
+ if body == nil {
+ return doRequestWithBodyFactory(ctx, apiName, addrs, route, method, nil)
+ }
+ bodyBytes, err := io.ReadAll(body)
+ if err != nil {
+ return nil, err
+ }
+ return doRequestWithBodyBytes(ctx, apiName, addrs, route, method, bodyBytes)
}📝 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.
| func doRequest(ctx context.Context, apiName string, addrs []string, route, method string, body io.Reader) ([]byte, error) { | |
| var bodyFactory func() io.Reader | |
| if body != nil { | |
| bodyFactory = func() io.Reader { | |
| return body | |
| } | |
| } | |
| return doRequestWithBodyFactory(ctx, apiName, addrs, route, method, bodyFactory) | |
| func doRequest(ctx context.Context, apiName string, addrs []string, route, method string, body io.Reader) ([]byte, error) { | |
| if body == nil { | |
| return doRequestWithBodyFactory(ctx, apiName, addrs, route, method, nil) | |
| } | |
| bodyBytes, err := io.ReadAll(body) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return doRequestWithBodyBytes(ctx, apiName, addrs, route, method, bodyBytes) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@domain/infosync/info.go` around lines 392 - 399, doRequest currently captures
and reuses the same io.Reader across retries, which causes later attempts to
read from a partially consumed reader; fix by reading the full body into a byte
slice if body != nil and call doRequestWithBodyBytes(ctx, apiName, addrs, route,
method, bodyBytes) so each retry gets a fresh reader (or otherwise construct a
new bytes.Reader per attempt); update the doRequest function to convert the
incoming body to bytes and delegate to doRequestWithBodyBytes instead of
creating a closure that returns the same io.Reader, referencing doRequest,
doRequestWithBodyFactory and doRequestWithBodyBytes to locate the change.
| func (do *Domain) loadOrInitMergeEmptyRegionsMinTableID() (int64, error) { | ||
| minTableID := mergeEmptyRegionsInitMinTableID | ||
| err := kv.RunInNewTxn(kv.WithInternalSourceType(context.Background(), kv.InternalTxnDDL), do.store, true, func(ctx context.Context, txn kv.Transaction) error { | ||
| t := meta.NewMeta(txn) | ||
| tableID, ok, err := t.GetMergeEmptyRegionsMinTableID() | ||
| if err != nil { | ||
| return errors.Trace(err) | ||
| } | ||
| if !ok || tableID < mergeEmptyRegionsInitMinTableID { | ||
| tableID = mergeEmptyRegionsInitMinTableID | ||
| if err := t.SetMergeEmptyRegionsMinTableID(tableID); err != nil { | ||
| return errors.Trace(err) | ||
| } | ||
| } | ||
| minTableID = tableID | ||
| return nil | ||
| }) | ||
| return minTableID, errors.Trace(err) |
There was a problem hiding this comment.
Thread the loop context into the checkpoint transactions.
doMergeEmptyRegions() is cancellation-aware, but these helpers replace the caller context with context.Background(). If meta KV is slow during shutdown, the loop can't abort the load/store/CAS phase and this goroutine can block domain teardown until those calls return.
💡 Suggested direction
-func (do *Domain) loadOrInitMergeEmptyRegionsMinTableID() (int64, error) {
+func (do *Domain) loadOrInitMergeEmptyRegionsMinTableID(ctx context.Context) (int64, error) {
minTableID := mergeEmptyRegionsInitMinTableID
- err := kv.RunInNewTxn(kv.WithInternalSourceType(context.Background(), kv.InternalTxnDDL), do.store, true, func(ctx context.Context, txn kv.Transaction) error {
+ err := kv.RunInNewTxn(kv.WithInternalSourceType(ctx, kv.InternalTxnDDL), do.store, true, func(ctx context.Context, txn kv.Transaction) error {
...
})
return minTableID, errors.Trace(err)
}
-func (do *Domain) storeMergeEmptyRegionsMinTableID(tableID int64) error {
+func (do *Domain) storeMergeEmptyRegionsMinTableID(ctx context.Context, tableID int64) error {
...
- return errors.Trace(kv.RunInNewTxn(kv.WithInternalSourceType(context.Background(), kv.InternalTxnDDL), do.store, true, func(ctx context.Context, txn kv.Transaction) error {
+ return errors.Trace(kv.RunInNewTxn(kv.WithInternalSourceType(ctx, kv.InternalTxnDDL), do.store, true, func(ctx context.Context, txn kv.Transaction) error {
return errors.Trace(meta.NewMeta(txn).SetMergeEmptyRegionsMinTableID(tableID))
}))
}
-func (do *Domain) storeMergeEmptyRegionsMinTableIDIfUnchanged(expectedTableID, nextTableID int64) (bool, error) {
+func (do *Domain) storeMergeEmptyRegionsMinTableIDIfUnchanged(ctx context.Context, expectedTableID, nextTableID int64) (bool, error) {
...
- err := kv.RunInNewTxn(kv.WithInternalSourceType(context.Background(), kv.InternalTxnDDL), do.store, true, func(ctx context.Context, txn kv.Transaction) error {
+ err := kv.RunInNewTxn(kv.WithInternalSourceType(ctx, kv.InternalTxnDDL), do.store, true, func(ctx context.Context, txn kv.Transaction) error {
...
})
return updated, errors.Trace(err)
}Also applies to: 124-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@domain/pkdb_force_merge.go` around lines 104 - 121, The helpers
loadOrInitMergeEmptyRegionsMinTableID and the similar
loadMergeEmptyRegionsMaxTableID replace the caller context with
context.Background(), blocking cancellation; change their signatures to accept a
context.Context parameter and use that ctx when calling
kv.WithInternalSourceType (i.e., replace context.Background() with the passed
ctx inside kv.RunInNewTxn), and update the caller doMergeEmptyRegions to pass
its ctx through so the checkpoint KV operations become cancellation-aware.
Ensure all call sites are updated to provide the context and adjust any error
handling signatures accordingly.
| // Partitioned tables can have duplicate _tidb_rowid between different partitions | ||
| // due to EXCHANGE PARTITION, if so, do not optimize skipping rows with multiple changes | ||
| skipMultipleChangesOnSameRow := true | ||
| tbl := e.tblID2table[content.TblID] | ||
| if _, ok := tbl.(table.PartitionedTable); ok { | ||
| if !tbl.Meta().HasClusteredIndex() { | ||
| skipMultipleChangesOnSameRow = false | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid globally disabling duplicate-row suppression for non-clustered partitioned tables
Line 119 forces e.changed to false for every repeated handle when skipMultipleChangesOnSameRow=false. That can re-run updates for the same physical row under join fan-out, violating the “update once per matched row” contract and risking inflated changed-row accounting. Please fix this via a partition-aware dedup key (physical partition identity + handle), instead of disabling dedup for the whole table class.
Based on learnings: "TiDB executor behavior for multi-table UPDATE ... Each matched row is updated once..." and "VirtualAssignmentsOffset semantics ... Executor applies ... only after the 'changed' check."
Also applies to: 119-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@executor/update.go` around lines 85 - 93, The current logic sets
skipMultipleChangesOnSameRow=false for all non-clustered PartitionedTable
instances, which globally disables dedup and causes repeated updates; instead,
implement a partition-aware dedup keyed by (physical partition ID + handle).
Change the branch that checks tbl.(table.PartitionedTable) /
tbl.Meta().HasClusteredIndex() to stop flipping a global flag and instead ensure
the update executor (where e.changed is cleared for repeated handles) consults a
new per-table map (e.g., seenHandlesByPartition) keyed by the partition identity
plus the row handle; mark entries when a physical (partitionID, handle) pair is
updated so subsequent matches for the same physical row skip setting
e.changed=false only if the same physical key was seen, leaving cross-partition
duplicates unaffected. Ensure the map is created/cleared per statement and
referenced where e.changed is evaluated for repeated handles.
| func TestSetTiDBEnableDropTableForceMerge(t *testing.T) { | ||
| vars := NewSessionVars(nil) | ||
| mock := NewMockGlobalAccessor4Tests() | ||
| mock.SessionVars = vars | ||
| vars.GlobalVarsAccessor = mock | ||
|
|
||
| oldValue := EnableDropTableForceMerge.Load() | ||
| EnableDropTableForceMerge.Store(DefTiDBEnableDropTableForceMerge) | ||
| defer EnableDropTableForceMerge.Store(oldValue) | ||
|
|
||
| sysVar := GetSysVar(TiDBEnableDropTableForceMerge) | ||
| require.Equal(t, Off, sysVar.Value) | ||
|
|
||
| require.NoError(t, mock.SetGlobalSysVar(context.Background(), TiDBEnableDropTableForceMerge, On)) | ||
| require.True(t, EnableDropTableForceMerge.Load()) | ||
|
|
||
| val, err := mock.GetGlobalSysVar(TiDBEnableDropTableForceMerge) | ||
| require.NoError(t, err) | ||
| require.Equal(t, On, val) | ||
|
|
||
| val, err = sysVar.GetGlobalFromHook(context.Background(), vars) | ||
| require.NoError(t, err) | ||
| require.Equal(t, On, val) | ||
|
|
||
| require.NoError(t, mock.SetGlobalSysVar(context.Background(), TiDBEnableDropTableForceMerge, Off)) | ||
| require.False(t, EnableDropTableForceMerge.Load()) | ||
|
|
||
| val, err = mock.GetGlobalSysVar(TiDBEnableDropTableForceMerge) | ||
| require.NoError(t, err) | ||
| require.Equal(t, Off, val) | ||
|
|
||
| val, err = sysVar.GetGlobalFromHook(context.Background(), vars) | ||
| require.NoError(t, err) | ||
| require.Equal(t, Off, val) | ||
| } |
There was a problem hiding this comment.
Add a SQL-level regression for the new sysvar.
This only verifies the in-process hook path. Please add a SQL integration test that sets tidb_enable_drop_table_force_merge and reads it back through SQL as well, so the user-visible behavior is covered end to end.
Based on learnings, "For session variables or protocol behavior changes, perform targeted package tests plus SQL integration tests for user-visible behavior."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sessionctx/variable/pkdb_force_merge_test.go` around lines 24 - 58, The unit
test only covers the in-process hook; add a SQL integration test named e.g.
TestSetTiDBEnableDropTableForceMerge_SQL that uses the SQL test harness
(TestKit) to SET the variable and read it back via SQL to cover the user-visible
path: execute "SET GLOBAL tidb_enable_drop_table_force_merge=ON" (and OFF), then
verify with "SELECT @@global.tidb_enable_drop_table_force_merge" and "SELECT
@@tidb_enable_drop_table_force_merge" (and session-level checks if appropriate),
and ensure cleanup/restore of the original value; reference the existing
TestSetTiDBEnableDropTableForceMerge, the sysvar TiDBEnableDropTableForceMerge
and the boolean flag EnableDropTableForceMerge when asserting effects.
| } | ||
|
|
||
| //go:linkname buildVersion runtime.buildVersion | ||
| // go:linkname buildVersion runtime.buildVersion |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type go '^\s*//\s+go:linkname\b|^\s*//go:linkname\b' \
util/printer/printer.go \
util/localpool/localpool.go \
util/fastrand/random.go \
br/pkg/lightning/manual/manual.goRepository: pingcap/tidb
Length of output: 410
Fix malformed compiler directive on Line 171: remove space between // and go:linkname.
The go:linkname directive requires the exact //go: prefix without spaces. The current // go:linkname will not be recognized by the compiler and breaks the link to runtime.buildVersion. All other files in the codebase use the correct //go:linkname format.
Proposed fix
-// go:linkname buildVersion runtime.buildVersion
+//go:linkname buildVersion runtime.buildVersion
var buildVersion string📝 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.
| // go:linkname buildVersion runtime.buildVersion | |
| //go:linkname buildVersion runtime.buildVersion |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@util/printer/printer.go` at line 171, The compiler directive on the line
containing the pseudo-symbol buildVersion is malformed: remove the space between
the comment slashes and the directive so the link directive uses the exact
"//go:linkname" prefix and links buildVersion to runtime.buildVersion; update
the line in printer.go where "// go:linkname buildVersion runtime.buildVersion"
appears to use the correct "//go:linkname" form so the compiler recognizes the
directive.
|
/test unit-test |
|
@HunDunDM: 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. |
1ef0449 to
34e2d44
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
domain/infosync/pkdb_force_merge.go (2)
31-35: Clarify exported key-range boundary semantics.
StartKey/EndKeyare exported, but the interval contract is implicit. A short field-level comment (inclusive/exclusive) will reduce caller ambiguity.As per coding guidelines: "Comments SHOULD explain non-obvious intent, constraints, invariants..."Proposed change
type ForceMergeKeyRange struct { - StartKey []byte - EndKey []byte + // StartKey is the inclusive lower bound of the force-merge range. + StartKey []byte + // EndKey is the exclusive upper bound of the force-merge range. + EndKey []byte }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@domain/infosync/pkdb_force_merge.go` around lines 31 - 35, The exported struct ForceMergeKeyRange has ambiguous boundary semantics for StartKey and EndKey; add concise field-level comments to StartKey and EndKey describing whether each bound is inclusive or exclusive (e.g., StartKey is inclusive, EndKey is exclusive), and document any sentinel meanings (nil/empty => unbounded) to remove caller ambiguity; update the struct's field comments next to StartKey and EndKey accordingly.
156-157: Use a log field name that matches the value semantics.Line 156 logs
batchIndex, but the value is 1-based (batchNumber). Rename tobatchNumber(or log zero-based index) to avoid confusion in log consumers.As per coding guidelines: "Code SHOULD be self-documenting through clear naming and structure."Proposed change
- zap.Int("batchIndex", batchNumber), + zap.Int("batchNumber", batchNumber),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@domain/infosync/pkdb_force_merge.go` around lines 156 - 157, The log field name is misleading: zap.Int("batchIndex", batchNumber) uses a 1-based batchNumber but names it batchIndex; update the zap.Int call to use a matching field name (e.g., zap.Int("batchNumber", batchNumber)) where this appears in domain/infosync/pkdb_force_merge.go (look for the zap.Int invocation that references batchNumber) so the log key matches the value semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@domain/infosync/pkdb_force_merge.go`:
- Around line 31-35: The exported struct ForceMergeKeyRange has ambiguous
boundary semantics for StartKey and EndKey; add concise field-level comments to
StartKey and EndKey describing whether each bound is inclusive or exclusive
(e.g., StartKey is inclusive, EndKey is exclusive), and document any sentinel
meanings (nil/empty => unbounded) to remove caller ambiguity; update the
struct's field comments next to StartKey and EndKey accordingly.
- Around line 156-157: The log field name is misleading: zap.Int("batchIndex",
batchNumber) uses a 1-based batchNumber but names it batchIndex; update the
zap.Int call to use a matching field name (e.g., zap.Int("batchNumber",
batchNumber)) where this appears in domain/infosync/pkdb_force_merge.go (look
for the zap.Int invocation that references batchNumber) so the log key matches
the value semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2106509c-b0f6-4752-8b02-e43fcc7c7168
📒 Files selected for processing (3)
domain/infosync/BUILD.bazeldomain/infosync/pkdb_force_merge.godomain/infosync/pkdb_force_merge_test.go
✅ Files skipped from review due to trivial changes (1)
- domain/infosync/pkdb_force_merge_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- domain/infosync/BUILD.bazel
|
/test mysql-test |
|
@HunDunDM: 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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-6.5-20231229-v6.5.6 #67725 +/- ##
================================================================
Coverage ? 73.6291%
================================================================
Files ? 1091
Lines ? 350239
Branches ? 0
================================================================
Hits ? 257878
Misses ? 75778
Partials ? 16583 🚀 New features to boost your workflow:
|
Signed-off-by: HunDunDM <hundundm@gmail.com>
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
domain/pkdb_force_merge.go (1)
112-166:⚠️ Potential issue | 🟠 MajorThread the caller
ctxinto the checkpoint transactions.
doMergeEmptyRegionsis cancellation-aware, but all three checkpoint helpers still hardcodecontext.Background(). A slow meta KV read/write can therefore outlive domain shutdown, andResetMergeEmptyRegionsMinTableID()cannot honor request cancellation either. Please pass the caller context through these helpers and use it inkv.WithInternalSourceType(...)instead ofcontext.Background().💡 Suggested direction
-func (do *Domain) doMergeEmptyRegions(ctx context.Context) error { - minTableID, err := do.loadOrInitMergeEmptyRegionsMinTableID() +func (do *Domain) doMergeEmptyRegions(ctx context.Context) error { + minTableID, err := do.loadOrInitMergeEmptyRegionsMinTableID(ctx) if err != nil { return errors.Trace(err) } ... - updated, err := do.storeMergeEmptyRegionsMinTableIDIfUnchanged(minTableID, maxTableID) + updated, err := do.storeMergeEmptyRegionsMinTableIDIfUnchanged(ctx, minTableID, maxTableID) ... } -func (do *Domain) loadOrInitMergeEmptyRegionsMinTableID() (int64, error) { +func (do *Domain) loadOrInitMergeEmptyRegionsMinTableID(ctx context.Context) (int64, error) { minTableID := mergeEmptyRegionsInitMinTableID - err := kv.RunInNewTxn(kv.WithInternalSourceType(context.Background(), kv.InternalTxnDDL), do.store, true, func(ctx context.Context, txn kv.Transaction) error { + err := kv.RunInNewTxn(kv.WithInternalSourceType(ctx, kv.InternalTxnDDL), do.store, true, func(ctx context.Context, txn kv.Transaction) error { ... }) return minTableID, errors.Trace(err) } -func (do *Domain) storeMergeEmptyRegionsMinTableID(tableID int64) error { +func (do *Domain) storeMergeEmptyRegionsMinTableID(ctx context.Context, tableID int64) error { ... - return errors.Trace(kv.RunInNewTxn(kv.WithInternalSourceType(context.Background(), kv.InternalTxnDDL), do.store, true, func(ctx context.Context, txn kv.Transaction) error { + return errors.Trace(kv.RunInNewTxn(kv.WithInternalSourceType(ctx, kv.InternalTxnDDL), do.store, true, func(ctx context.Context, txn kv.Transaction) error { return errors.Trace(meta.NewMeta(txn).SetMergeEmptyRegionsMinTableID(tableID)) })) } -func (do *Domain) storeMergeEmptyRegionsMinTableIDIfUnchanged(expectedTableID, nextTableID int64) (bool, error) { +func (do *Domain) storeMergeEmptyRegionsMinTableIDIfUnchanged(ctx context.Context, expectedTableID, nextTableID int64) (bool, error) { ... - err := kv.RunInNewTxn(kv.WithInternalSourceType(context.Background(), kv.InternalTxnDDL), do.store, true, func(ctx context.Context, txn kv.Transaction) error { + err := kv.RunInNewTxn(kv.WithInternalSourceType(ctx, kv.InternalTxnDDL), do.store, true, func(ctx context.Context, txn kv.Transaction) error { ... }) return updated, errors.Trace(err) }Also applies to: 171-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@domain/pkdb_force_merge.go` around lines 112 - 166, The helper functions loadOrInitMergeEmptyRegionsMinTableID, storeMergeEmptyRegionsMinTableID, and storeMergeEmptyRegionsMinTableIDIfUnchanged must accept and use the caller's ctx instead of always using context.Background(); add a context.Context parameter to each function signature, replace kv.WithInternalSourceType(context.Background(), ...) with kv.WithInternalSourceType(ctx, ...), and propagate that ctx from callers (e.g., doMergeEmptyRegions and ResetMergeEmptyRegionsMinTableID) so checkpoint transactions respect cancellation/timeouts. Ensure all call sites are updated to pass the caller ctx and keep existing behavior (clamping tableID to mergeEmptyRegionsInitMinTableID) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ddl/delete_range.go`:
- Around line 275-292: The code currently lets
GetForceMergeRangesForGCDeleteRange mutate gcForceMergeTableCache before calling
infosync.AddForceMergeRanges, causing a logical-table entry to be cached even if
AddForceMergeRanges fails; change the flow in delRange.reportForceMergeRanges so
that gcForceMergeTableCache is only updated after a successful
infosync.AddForceMergeRanges call (or, alternatively, make
GetForceMergeRangesForGCDeleteRange return the ranges without mutating the cache
and then update gcForceMergeTableCache inside reportForceMergeRanges only when
AddForceMergeRanges returns nil); reference the functions
delRange.reportForceMergeRanges, GetForceMergeRangesForGCDeleteRange, the
gcForceMergeTableCache map, and infosync.AddForceMergeRanges when making this
change.
---
Duplicate comments:
In `@domain/pkdb_force_merge.go`:
- Around line 112-166: The helper functions
loadOrInitMergeEmptyRegionsMinTableID, storeMergeEmptyRegionsMinTableID, and
storeMergeEmptyRegionsMinTableIDIfUnchanged must accept and use the caller's ctx
instead of always using context.Background(); add a context.Context parameter to
each function signature, replace kv.WithInternalSourceType(context.Background(),
...) with kv.WithInternalSourceType(ctx, ...), and propagate that ctx from
callers (e.g., doMergeEmptyRegions and ResetMergeEmptyRegionsMinTableID) so
checkpoint transactions respect cancellation/timeouts. Ensure all call sites are
updated to pass the caller ctx and keep existing behavior (clamping tableID to
mergeEmptyRegionsInitMinTableID) unchanged.
🪄 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: f4c97b6b-141c-42f9-acfd-f5c66dde84e6
📒 Files selected for processing (2)
ddl/delete_range.godomain/pkdb_force_merge.go
| func (dr *delRange) reportForceMergeRanges(ctx context.Context, sctx sessionctx.Context, r util.DelRangeTask, gcForceMergeTableCache map[int64]struct{}) error { | ||
| if !variable.EnableDropTableForceMerge.Load() { | ||
| return nil | ||
| } | ||
|
|
||
| historyJob, err := GetHistoryJobByID(sctx, r.JobID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if historyJob == nil { | ||
| return errors.Errorf("ddl job %d not found", r.JobID) | ||
| } | ||
|
|
||
| forceMergeRanges := GetForceMergeRangesForGCDeleteRange(historyJob, r, gcForceMergeTableCache) | ||
| if len(forceMergeRanges) == 0 { | ||
| return nil | ||
| } | ||
| return infosync.AddForceMergeRanges(ctx, forceMergeRanges) |
There was a problem hiding this comment.
Don't mark the logical-table range as sent before PD accepts it.
GetForceMergeRangesForGCDeleteRange(...) updates gcForceMergeTableCache inside ddl/pkdb_force_merge.go:100-120 before infosync.AddForceMergeRanges(...) runs. If the first partition task for a dropped/truncated partitioned table hits a transient PD error here, the table ID stays cached and the remaining partition tasks in the same doDelRangeWork pass will skip the logical-table/global-index range entirely. That turns one failed dispatch into a permanent missed force-merge report for the logical table.
Only commit the dedupe-cache entry after a successful send, or roll it back on error.
💡 Suggested direction
func (dr *delRange) reportForceMergeRanges(ctx context.Context, sctx sessionctx.Context, r util.DelRangeTask, gcForceMergeTableCache map[int64]struct{}) error {
if !variable.EnableDropTableForceMerge.Load() {
return nil
}
historyJob, err := GetHistoryJobByID(sctx, r.JobID)
if err != nil {
return err
}
if historyJob == nil {
return errors.Errorf("ddl job %d not found", r.JobID)
}
+ cacheSize := len(gcForceMergeTableCache)
forceMergeRanges := GetForceMergeRangesForGCDeleteRange(historyJob, r, gcForceMergeTableCache)
if len(forceMergeRanges) == 0 {
return nil
}
- return infosync.AddForceMergeRanges(ctx, forceMergeRanges)
+ if err := infosync.AddForceMergeRanges(ctx, forceMergeRanges); err != nil {
+ if len(gcForceMergeTableCache) != cacheSize {
+ delete(gcForceMergeTableCache, historyJob.TableID)
+ }
+ return err
+ }
+ return nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ddl/delete_range.go` around lines 275 - 292, The code currently lets
GetForceMergeRangesForGCDeleteRange mutate gcForceMergeTableCache before calling
infosync.AddForceMergeRanges, causing a logical-table entry to be cached even if
AddForceMergeRanges fails; change the flow in delRange.reportForceMergeRanges so
that gcForceMergeTableCache is only updated after a successful
infosync.AddForceMergeRanges call (or, alternatively, make
GetForceMergeRangesForGCDeleteRange return the ranges without mutating the cache
and then update gcForceMergeTableCache inside reportForceMergeRanges only when
AddForceMergeRanges returns nil); reference the functions
delRange.reportForceMergeRanges, GetForceMergeRangesForGCDeleteRange, the
gcForceMergeTableCache map, and infosync.AddForceMergeRanges when making this
change.
|
/test mysql-test |
|
@HunDunDM: 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. |
4 participants
Only for CI
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
New Features
Bug Fixes