schemastore: stop GC keeper for tombstone keyspace - #6159
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe schema store GC keeper now stops its refresh loop when PD reports a non-classic keyspace as ChangesGC keeper tombstone handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The GC keeper now stops refreshing safepoints after PD confirms a keyspace is tombstoned, preventing repeated requests and warnings for deleted keyspaces while retaining retry behavior for transient failures. No current merge-blocking risk is identified. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The PR satisfies the requirement to stop indefinite safepoint-refresh retries after PD reports TOMBSTONE, and it preserves retry behavior for other failures [ Resolution Implement keyspace schema-store cleanup and unregistration after no changefeed or local consumer remains, using reference counting or another explicit lifecycle mechanism that supports multiple changefeeds and per-node consumers. Alternatively, split or revise the linked issue if that broader cleanup is intentionally out of scope for this PR.
✨ 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 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 3AceShowHand, lidezhu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
|
@asddongmen: The following tests 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. |
What problem does this PR solve?
Issue Number: close #6156
After PD tombstones a keyspace, its per-node schema-store GC keeper keeps trying to refresh the keyspace safepoint every two minutes. PD rejects every request, but the keeper treats the failure as retryable forever, causing useless requests and recurring warnings until TiCDC restarts.
What is changed and how it works?
After a NextGen safepoint refresh fails, the keeper loads the keyspace metadata from PD. If PD confirms the irreversible
TOMBSTONEstate, the keeper logs the terminal lifecycle event once and exits its refresh goroutine.Successful refreshes do not add a metadata request. Enabled keyspaces and metadata lookup failures retain the existing retry behavior. A table-driven unit test covers all four paths.
Check List
Tests
Questions
Will it cause performance regression or break compatibility?
No. The additional PD request is made only after a failed NextGen safepoint refresh. Classic mode and retryable failures are unchanged.
Do you need to update user documentation, design documentation or monitoring documentation?
No. This fixes an internal lifecycle error path without changing user-facing configuration or APIs.
Release note
Summary by CodeRabbit