gc etcd key use meta service group#10949
Conversation
Signed-off-by: ystaticy <y_static_y@sina.com>
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
dco-signoff: yes
|
[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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds meta-service-group-aware txn safe point routing to the GC subsystem. A new ChangesMeta-service group GC txn safe point routing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 |
ti-chi-bot
Bot
added
the
size/XXL
Signed-off-by: ystaticy <y_static_y@sina.com>
ti-chi-bot
Bot
added
size/XL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/gc/gc_state_manager.go (1)
469-484: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftPreserve the local GC-state revision guard when txn safe points are routed.
The routed transaction now commits against the meta-service-group storage revision, but the blockers are still read from local GC state (
LoadGCSafePoint,LoadAllGCBarriers,CompatibleLoadTiDBMinStartTS,LoadAllGlobalGCBarriers). A concurrent local barrier write won’t conflict with the remote transaction, so the txn safe point can advance past a newly added blocker; expired global-barrier deletes are also enqueued against the remote storage.Please keep blocker reads/writes under a local GC-state revision check, or move the relevant blocker state into the same routed storage before advancing the routed txn safe point.
Also applies to: 535-544, 557-563
🤖 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/gc/gc_state_manager.go` around lines 469 - 484, The routed txn safe point update in the GC state flow is bypassing the local GC-state revision guard, so blocker reads and related writes can race with concurrent local GC state changes. In the GCStateManager path around the txn safe point transaction, keep the blocker checks and updates (LoadGCSafePoint, LoadAllGCBarriers, CompatibleLoadTiDBMinStartTS, LoadAllGlobalGCBarriers) protected by the local revision validation, or move that blocker state into the same routed storage used by the transaction before advancing the safe point. Make the same adjustment in the other affected GC-state methods so routed writes cannot skip newly added blockers.
🧹 Nitpick comments (1)
pkg/storage/endpoint/gc_states_test.go (1)
713-780: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd coverage for routed advancement with concurrent/local blockers.
The test verifies read/write placement, but not the GC manager path where txn safe points are routed while GC barriers remain local. Please add a case that exercises
AdvanceTxnSafePointwith local barriers/global barriers to catch revision-guard regressions.🤖 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/storage/endpoint/gc_states_test.go` around lines 713 - 780, The current test only verifies where txn safe points are stored, but it does not cover routed advancement through the GC manager when local and global GC barriers are present. Extend TestTxnSafePointWithMetaServiceGroups to exercise AdvanceTxnSafePoint on the GC state provider and include a case with local barriers/global barriers so revision-guard behavior is validated. Use the existing localStorage, group1Storage, group2Storage, and provider setup to assert the advancement path and resulting safe point values.
🤖 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 `@client/client.go`:
- Around line 569-606: Use a classic counted retry loop in the GetTSAsync retry
path so the final retry count is preserved for the post-loop check; the current
`for retryCount = range maxRetries` form leaves `retryCount` at `maxRetries-1`
after exhaustion, which prevents `checkRetry` from firing as intended. Update
the loop in the `GetTSAsync` retry logic so `retryCount` reaches `maxRetries`
when all attempts are used, while keeping the existing retryable error handling,
logging, and failpoint behavior unchanged.
In `@client/clients/tso/client.go`:
- Around line 1041-1043: The failpoint block in the health check path mutates
resp.Status before confirming resp is non-nil, which can panic when the health
check already failed. Update the logic around the failpoint in the health-check
handler to guard resp before setting its Status, using the existing health-check
response handling in the same function so the mutation only happens when a
response object is available.
In `@pkg/storage/endpoint/gc_states.go`:
- Around line 499-508: `RunInTxnSafePointTransaction` is still handing callers a
full `GCStateWriteBatch`, which lets routed txn-safe-point code write
non-txn-safe-point GC state on the meta-service-group storage. Update
`GCStateProvider.RunInTxnSafePointTransaction` and the related
`runInGCStateTransactionOnStorage`/`advanceTxnSafePointImpl` call path so this
API only permits txn-safe-point writes, either by narrowing the batch surface or
validating/rejecting barrier and global-barrier operations before execution.
In `@pkg/storage/endpoint/meta_service_group_gc_storage.go`:
- Around line 134-136: The stale etcd client cleanup in the group endpoint swap
logic is ignoring Close() failures, which violates the errcheck guideline and
can hide resource issues. Update the old.client.Close() call in the meta service
group GC storage path to handle and surface the error instead of discarding it,
using the existing group/client cleanup flow around old.client and the endpoint
update logic so failures are logged or propagated appropriately.
---
Outside diff comments:
In `@pkg/gc/gc_state_manager.go`:
- Around line 469-484: The routed txn safe point update in the GC state flow is
bypassing the local GC-state revision guard, so blocker reads and related writes
can race with concurrent local GC state changes. In the GCStateManager path
around the txn safe point transaction, keep the blocker checks and updates
(LoadGCSafePoint, LoadAllGCBarriers, CompatibleLoadTiDBMinStartTS,
LoadAllGlobalGCBarriers) protected by the local revision validation, or move
that blocker state into the same routed storage used by the transaction before
advancing the safe point. Make the same adjustment in the other affected
GC-state methods so routed writes cannot skip newly added blockers.
---
Nitpick comments:
In `@pkg/storage/endpoint/gc_states_test.go`:
- Around line 713-780: The current test only verifies where txn safe points are
stored, but it does not cover routed advancement through the GC manager when
local and global GC barriers are present. Extend
TestTxnSafePointWithMetaServiceGroups to exercise AdvanceTxnSafePoint on the GC
state provider and include a case with local barriers/global barriers so
revision-guard behavior is validated. Use the existing localStorage,
group1Storage, group2Storage, and provider setup to assert the advancement path
and resulting safe point values.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec022aff-c37c-43ed-93d0-df1322ada79a
📒 Files selected for processing (9)
client/client.goclient/clients/tso/client.gopkg/gc/gc_state_manager.gopkg/keyspace/keyspace.gopkg/keyspace/meta_service_group.gopkg/storage/endpoint/gc_states.gopkg/storage/endpoint/gc_states_test.gopkg/storage/endpoint/meta_service_group_gc_storage.goserver/server.go
| if _, _err_ := failpoint.Eval(_curpkg_("unreachableNetwork")); _err_ == nil { | ||
| resp.Status = healthpb.HealthCheckResponse_UNKNOWN | ||
| }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard resp before mutating failpoint state.
This failpoint runs before err is checked, so an errored health check with a nil response will panic on resp.Status.
Proposed fix
resp, err := healthCli.Check(healthCtx, &healthpb.HealthCheckRequest{Service: ""})
if _, _err_ := failpoint.Eval(_curpkg_("unreachableNetwork")); _err_ == nil {
+ if resp == nil {
+ resp = &healthpb.HealthCheckResponse{}
+ }
resp.Status = healthpb.HealthCheckResponse_UNKNOWN
}📝 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_ := failpoint.Eval(_curpkg_("unreachableNetwork")); _err_ == nil { | |
| resp.Status = healthpb.HealthCheckResponse_UNKNOWN | |
| }) | |
| } | |
| resp, err := healthCli.Check(healthCtx, &healthpb.HealthCheckRequest{Service: ""}) | |
| if _, _err_ := failpoint.Eval(_curpkg_("unreachableNetwork")); _err_ == nil { | |
| if resp == nil { | |
| resp = &healthpb.HealthCheckResponse{} | |
| } | |
| resp.Status = healthpb.HealthCheckResponse_UNKNOWN | |
| } |
🤖 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 `@client/clients/tso/client.go` around lines 1041 - 1043, The failpoint block
in the health check path mutates resp.Status before confirming resp is non-nil,
which can panic when the health check already failed. Update the logic around
the failpoint in the health-check handler to guard resp before setting its
Status, using the existing health-check response handling in the same function
so the mutation only happens when a response object is available.
| func (p GCStateProvider) RunInTxnSafePointTransaction(keyspaceID uint32, f func(GCStateProvider, *GCStateWriteBatch) error) error { | ||
| storage, err := p.getTxnSafePointStorage(keyspaceID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| txnSafePointProvider := NewGCStateProvider(storage) | ||
| return runInGCStateTransactionOnStorage(storage, func(wb *GCStateWriteBatch) error { | ||
| return f(txnSafePointProvider, wb) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Keep routed txn-safe-point transactions scoped to txn-safe-point data.
RunInTxnSafePointTransaction still exposes a full GCStateWriteBatch, so callers can enqueue GC barrier/global barrier writes that will execute on the routed meta-service-group storage instead of the local GC-state storage. This is already easy to hit from advanceTxnSafePointImpl; consider narrowing this API or validating allowed ops.
🤖 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/storage/endpoint/gc_states.go` around lines 499 - 508,
`RunInTxnSafePointTransaction` is still handing callers a full
`GCStateWriteBatch`, which lets routed txn-safe-point code write
non-txn-safe-point GC state on the meta-service-group storage. Update
`GCStateProvider.RunInTxnSafePointTransaction` and the related
`runInGCStateTransactionOnStorage`/`advanceTxnSafePointImpl` call path so this
API only permits txn-safe-point writes, either by narrowing the batch surface or
validating/rejecting barrier and global-barrier operations before execution.
| if old != nil && old.client != nil { | ||
| _ = old.client.Close() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle the stale etcd client close error.
Line 135 ignores Close() failures, which can hide cleanup/resource issues when group endpoints change.
Proposed fix
if old != nil && old.client != nil {
- _ = old.client.Close()
+ if err := old.client.Close(); err != nil {
+ return nil, errors.Wrapf(err, "close stale meta-service group %s etcd client", groupID)
+ }
}As per coding guidelines, “No ignored errors unless allowed in .golangci.yml errcheck exclusions.”
📝 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 old != nil && old.client != nil { | |
| _ = old.client.Close() | |
| } | |
| if old != nil && old.client != nil { | |
| if err := old.client.Close(); err != nil { | |
| return nil, errors.Wrapf(err, "close stale meta-service group %s etcd client", groupID) | |
| } | |
| } |
🤖 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/storage/endpoint/meta_service_group_gc_storage.go` around lines 134 -
136, The stale etcd client cleanup in the group endpoint swap logic is ignoring
Close() failures, which violates the errcheck guideline and can hide resource
issues. Update the old.client.Close() call in the meta service group GC storage
path to handle and surface the error instead of discarding it, using the
existing group/client cleanup flow around old.client and the endpoint update
logic so failures are logged or propagated appropriately.
Source: Coding guidelines
| if err != nil { | ||
| return AdvanceTxnSafePointResult{}, err | ||
| } | ||
| err = m.gcMetaStorage.RunInTxnSafePointTransaction(keyspaceID, func(txnSafePointProvider endpoint.GCStateProvider, wb *endpoint.GCStateWriteBatch) error { |
There was a problem hiding this comment.
This transaction writes the routed txn safe point in remote etcd, but still reads local GC barriers, global barriers, and min-start-ts. Local blocker changes will not conflict with the remote revision, so the txn safe point can advance past a new blocker.
| newTxnSafePoint = max(oldTxnSafePoint, minBlocker) | ||
|
|
||
| if downgradeCompatibleMode { | ||
| if downgradeCompatibleMode && !isTxnSafePointRouted { |
There was a problem hiding this comment.
This routed guard is also needed for the expired global barrier cleanup above. Otherwise the routed batch deletes from remote storage while the global barriers were read from local storage.
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Linking issues" section in the CONTRIBUTING.md. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10949 +/- ##
==========================================
- Coverage 79.26% 79.11% -0.15%
==========================================
Files 540 542 +2
Lines 74954 75457 +503
==========================================
+ Hits 59412 59700 +288
- Misses 11371 11501 +130
- Partials 4171 4256 +85
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
2 participants
What problem does this PR solve?
Issue Number: Close #xxx
What is changed and how does it work?
Check List
Tests
Code changes
Side effects
Related changes
pingcap/docs/pingcap/docs-cn:pingcap/tiup:Release note
Summary by CodeRabbit
New Features
Bug Fixes
Tests