[#1][TSO/Keyspace Group] Remove archive/tombstone keyspace from group (407ad06f)#10556
[#1][TSO/Keyspace Group] Remove archive/tombstone keyspace from group (407ad06f)#10556bufferflies wants to merge 3 commits intotikv:masterfrom
Conversation
…ikv#420) * support remove archive & tombstone keyspace from keyspace group Signed-off-by: ystaticy <y_static_y@sina.com> * support keyspace list Signed-off-by: ystaticy <y_static_y@sina.com> * no keyspace to remove ,return succ Signed-off-by: ystaticy <y_static_y@sina.com> * remove debug log Signed-off-by: ystaticy <y_static_y@sina.com> --------- Signed-off-by: ystaticy <y_static_y@sina.com> (cherry picked from commit 407ad06)
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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. |
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 |
|
/ping @bufferflies |
ti-chi-bot
bot
added
the
size/XL
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a service method and HTTP DELETE endpoint to remove archived or tombstone keyspaces from a keyspace group; performs validation, filters non-removable IDs, persists changes via an etcd transaction only when needed, and updates the in-memory groups cache. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as HTTP Handler
participant KeyspaceMgr as KeyspaceManager
participant GroupMgr as GroupManager
participant Etcd as etcd/Storage
participant Cache as In-Memory Cache
Client->>Handler: DELETE /tso/keyspace-groups/:id/keyspaces\n{keyspaces: [...]}
Handler->>Handler: Validate group ID and body
Handler->>KeyspaceMgr: Load each keyspace metadata
KeyspaceMgr-->>Handler: Metadata (state or not found)
Handler->>Handler: Filter to ARCHIVED/TOMBSTONE (skip missing)
alt No valid keyspaces
Handler->>GroupMgr: GetKeyspaceGroupByID(groupID)
GroupMgr-->>Handler: Current KeyspaceGroup
Handler-->>Client: 200 OK {KeyspaceGroup}
else Has valid keyspaces
Handler->>GroupMgr: RemoveKeyspacesFromGroup(groupID, validIDs)
GroupMgr->>GroupMgr: Acquire write lock
GroupMgr->>Etcd: Load KeyspaceGroup (txn)
Etcd-->>GroupMgr: KeyspaceGroup or not found
GroupMgr->>GroupMgr: Build removal set (skip default/non-members)
alt Removals needed
GroupMgr->>Etcd: Save updated KeyspaceGroup (txn)
Etcd-->>GroupMgr: Success
GroupMgr->>Cache: groups[userKind].Put(kg)
else No changes required
GroupMgr-->>GroupMgr: Skip persistence
end
GroupMgr-->>Handler: Updated KeyspaceGroup
Handler-->>Client: 200 OK {KeyspaceGroup}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/keyspace/tso_keyspace_group.go`:
- Around line 427-487: In RemoveKeyspacesFromGroup, before building toRemove add
the same split/merge guards used elsewhere: after loading kg validate
kg.IsSplitting() and kg.IsMerging() and return
errs.ErrKeyspaceGroupInSplit.FastGenByArgs(groupID) or
errs.ErrKeyspaceGroupInMerging.FastGenByArgs(groupID) respectively; ensure these
checks occur right after the kg nil check (using kg and groupID) so the function
aborts mutations during split/merge just like SetNodesForKeyspaceGroup,
UpdateKeyspaceForGroup, MoveKeyspacesToGroup, DeleteKeyspaceGroupByID, and
MergeKeyspaceGroups.
In `@server/apiv2/handlers/tso_keyspace_group.go`:
- Around line 581-584: The JSON bind error response uses
errs.ErrBindJSON.Wrap(err).GenWithStackByCause() (an error object) directly in
c.AbortWithStatusJSON after c.BindJSON(¶ms); update the call to pass a
string by invoking .Error() on the generated error (i.e., use
errs.ErrBindJSON.Wrap(err).GenWithStackByCause().Error()) so the payload
serializes consistently like other handlers; modify the code path that calls
c.AbortWithStatusJSON to send the .Error() string.
- Around line 625-633: When validKeyspaces is empty the handler calls
groupManager.GetKeyspaceGroupByID and returns kg without checking for nil;
update the handler to check if kg == nil after calling GetKeyspaceGroupByID and
return an error (e.g., c.AbortWithStatusJSON with ErrKeyspaceGroupNotExists or
appropriate HTTP 404/500) instead of returning HTTP 200 with null. Ensure you
reference the existing symbols: validKeyspaces,
groupManager.GetKeyspaceGroupByID, kg, and ErrKeyspaceGroupNotExists when
implementing the nil-check and error response.
In `@tests/server/apiv2/handlers/tso_keyspace_group_test.go`:
- Around line 276-277: The test enables two failpoints via
failpoint.Enable("github.com/tikv/pd/server/delayStartServerLoop",
`return(true)`) and
failpoint.Enable("github.com/tikv/pd/pkg/keyspace/skipSplitRegion",
"return(true)") but never disables them; add deferred cleanup immediately after
each successful enable: call defer
re.NoError(failpoint.Disable("github.com/tikv/pd/server/delayStartServerLoop"))
and defer
re.NoError(failpoint.Disable("github.com/tikv/pd/pkg/keyspace/skipSplitRegion"))
so both failpoints are disabled when the test exits.
🪄 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: dcb16c05-d4ad-4596-9132-2f4b74e702ec
📒 Files selected for processing (4)
pkg/keyspace/tso_keyspace_group.goserver/apiv2/handlers/tso_keyspace_group.gotests/server/apiv2/handlers/testutil.gotests/server/apiv2/handlers/tso_keyspace_group_test.go
|
/ping @ystaticy |
Signed-off-by: bufferflies <1045931706@qq.com>
|
Re-reviewed the latest follow-up on head This delta looks clean to me. The unresolved review items in the keyspace-group removal path and the related test cleanup are addressed, and I do not see a new behavior regression introduced by this follow-up. I do not have a new finding on this delta. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/keyspace/tso_keyspace_group.go`:
- Around line 455-460: The loop that builds toRemove currently only skips
constant.DefaultKeyspaceID and can thus remove the bootstrap/system keyspace;
update the check in the keyspaceIDs iteration (where toRemove is populated) to
also skip the protected bootstrap keyspace by using the existing helpers: call
GetBootstrapKeyspaceID() or isProtectedKeyspaceID(ksID) and treat matches as
exempt from removal (i.e., continue the loop when ksID ==
constant.DefaultKeyspaceID || ksID == GetBootstrapKeyspaceID() ||
isProtectedKeyspaceID(ksID)) so the protected member of the default group is
never removed.
In `@tests/server/apiv2/handlers/tso_keyspace_group_test.go`:
- Around line 276-279: The defer calls currently call failpoint.Disable
immediately because re.NoError(...) is invoked at defer time; change the defer
usage to defer a closure that calls re.NoError(failpoint.Disable(...)) so the
Disable calls run at function exit. Locate the two places where
failpoint.Enable("github.com/tikv/pd/server/delayStartServerLoop", ...) and
failpoint.Enable("github.com/tikv/pd/pkg/keyspace/skipSplitRegion", ...) are
used and replace their corresponding defer re.NoError(failpoint.Disable(...))
with defer func() {
re.NoError(failpoint.Disable("github.com/tikv/pd/server/delayStartServerLoop"))
}() and defer func() {
re.NoError(failpoint.Disable("github.com/tikv/pd/pkg/keyspace/skipSplitRegion"))
}() respectively so the disables execute only when the test exits.
🪄 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: a5b58eb4-909d-4901-87ab-efa363515454
📒 Files selected for processing (3)
pkg/keyspace/tso_keyspace_group.goserver/apiv2/handlers/tso_keyspace_group.gotests/server/apiv2/handlers/tso_keyspace_group_test.go
| // Build a set of keyspaces to remove (excluding default keyspace) | ||
| toRemove := make(map[uint32]struct{}) | ||
| for _, ksID := range keyspaceIDs { | ||
| // Skip default keyspace | ||
| if ksID == constant.DefaultKeyspaceID { | ||
| continue |
There was a problem hiding this comment.
Protect the bootstrap/system keyspace here too.
This branch only skips constant.DefaultKeyspaceID, but the rest of this file treats the protected member of the default group as the bootstrap keyspace via GetBootstrapKeyspaceID() / isProtectedKeyspaceID(). In deployments where the bootstrap keyspace is the system keyspace, this path can remove it from the default group and break that invariant.
Suggested fix
- if ksID == constant.DefaultKeyspaceID {
+ if isProtectedKeyspaceID(ksID) {
continue
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/keyspace/tso_keyspace_group.go` around lines 455 - 460, The loop that
builds toRemove currently only skips constant.DefaultKeyspaceID and can thus
remove the bootstrap/system keyspace; update the check in the keyspaceIDs
iteration (where toRemove is populated) to also skip the protected bootstrap
keyspace by using the existing helpers: call GetBootstrapKeyspaceID() or
isProtectedKeyspaceID(ksID) and treat matches as exempt from removal (i.e.,
continue the loop when ksID == constant.DefaultKeyspaceID || ksID ==
GetBootstrapKeyspaceID() || isProtectedKeyspaceID(ksID)) so the protected member
of the default group is never removed.
Signed-off-by: bufferflies <1045931706@qq.com>
|
Re-reviewed the latest cleanup follow-up on head This delta looks clean to me. The failpoint cleanup in I do not have a new finding on this delta. |
|
@bufferflies: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Linking issues" section in the CONTRIBUTING.md. |
2 participants
Summary
Issue Number
ref #10516, close #10555
author: @ystaticy
cp
407ad06fChecklist
mastermake checkSummary by CodeRabbit
New Features
Tests