meta-service-group: change add method to patch#10881
Conversation
* change to patch Signed-off-by: AmoebaProtozoa <8039876+AmoebaProtozoa@users.noreply.github.com> * fix swagger doc Signed-off-by: AmoebaProtozoa <8039876+AmoebaProtozoa@users.noreply.github.com> --------- Signed-off-by: AmoebaProtozoa <8039876+AmoebaProtozoa@users.noreply.github.com> (cherry picked from commit 0b70d5d) Signed-off-by: bufferflies <1045931706@qq.com>
ti-chi-bot
Bot
added
release-note-none
|
@coderabbitai full review |
|
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:
📝 WalkthroughWalkthroughReplaces the ChangesMeta-service-groups PATCH API Migration
Sequence Diagram(s)sequenceDiagram
participant Client
participant PatchMetaServiceGroups
participant UpdateGroupsSafely
participant Storage
participant SetKeyspaceConfigWithoutKeyspaceManagerUpdate
participant UpdateKeyspaceConfig
Client->>PatchMetaServiceGroups: PATCH /meta-service-groups (map[string]*string)
PatchMetaServiceGroups->>PatchMetaServiceGroups: validate IDs/addresses, build newGroups + deletedGroups
PatchMetaServiceGroups->>UpdateGroupsSafely: (ctx, newGroups, deletedGroups, persist, afterPersist)
UpdateGroupsSafely->>UpdateGroupsSafely: acquire exclusive lock
UpdateGroupsSafely->>Storage: load assignment counts per group
Storage-->>UpdateGroupsSafely: counts
alt group has assigned keyspaces
UpdateGroupsSafely-->>PatchMetaServiceGroups: error "cannot delete meta-service group …"
PatchMetaServiceGroups-->>Client: 400 Bad Request
else all deletions safe
UpdateGroupsSafely->>SetKeyspaceConfigWithoutKeyspaceManagerUpdate: persist cloned config (etcd)
UpdateGroupsSafely->>UpdateGroupsSafely: update in-memory metaServiceGroups map, release lock
UpdateGroupsSafely->>UpdateKeyspaceConfig: afterPersist: apply config to keyspaceManager
UpdateGroupsSafely-->>PatchMetaServiceGroups: nil
PatchMetaServiceGroups-->>Client: 200 []MetaServiceGroupStatus (sorted by ID)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
ti-chi-bot
Bot
added
the
size/L
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/server/apiv2/handlers/meta_service_group_test.go (1)
58-58: 💤 Low valueClarify why region split wait is disabled.
This line disables region split waiting during test setup, but the relationship to PATCH migration is unclear. If this is a test performance optimization, consider documenting it with an inline comment.
🤖 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 `@tests/server/apiv2/handlers/meta_service_group_test.go` at line 58, conf.Keyspace.WaitRegionSplit is being set to false with no explanation; add an inline comment next to the assignment (conf.Keyspace.WaitRegionSplit = false) stating why region split waiting is disabled (e.g., test performance/stability and not related to PATCH migration) and if relevant note any risk or how to enable it for debugging to clarify intent for future readers.
🤖 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 `@server/apiv2/handlers/meta_service_group.go`:
- Around line 71-89: Validate patch keys and values before mutating newGroups:
after binding into patch (map[string]*string) iterate the entries and for any
key that is empty or only whitespace (use strings.TrimSpace) or any non-nil
value that is empty/whitespace, return a 400 Bad Request (or respond with
errs.ErrBindJSON.Wrap(...) style) instead of applying the change; only allow
delete when key is valid and value is nil, and only apply add/update when both
key and *value are non-empty trimmed strings. Import "strings" for the TrimSpace
check and perform this validation before using manager.GetGroups()/maps.Copy and
before mutating newGroups.
In `@tools/pd-ctl/pdctl/command/meta_service_group_command.go`:
- Around line 121-125: In newDeleteMetaServiceGroupFunc, validate each arg after
trimming (use trimmed := strings.TrimSpace(id)) and reject empty values instead
of inserting them into patch; if trimmed == "" then report a user-facing error
(via the Cobra command context/printing) and abort the command so no invalid
empty-key entry is sent to the server; only add non-empty trimmed IDs to the
patch map.
- Around line 87-96: The loop building patch from metaServiceGroups must
validate that both the trimmed key and value are non-empty: after splitting with
strings.SplitN(group, "=", 2) and trimming, check that
strings.TrimSpace(parts[0]) is not empty (group ID) and that addr (the trimmed
parts[1]) is not empty; if either is empty, call cmd.PrintErrf with a helpful
message like "Invalid --group format: %q (empty id or address)" and return
instead of inserting into patch; update the loop that assigns
patch[strings.TrimSpace(parts[0])] = &addr to perform these checks before
assignment.
---
Nitpick comments:
In `@tests/server/apiv2/handlers/meta_service_group_test.go`:
- Line 58: conf.Keyspace.WaitRegionSplit is being set to false with no
explanation; add an inline comment next to the assignment
(conf.Keyspace.WaitRegionSplit = false) stating why region split waiting is
disabled (e.g., test performance/stability and not related to PATCH migration)
and if relevant note any risk or how to enable it for debugging to clarify
intent for future readers.
🪄 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: 8c482a70-6bd2-41d5-82b8-6e05090e9a6f
📒 Files selected for processing (4)
server/apiv2/handlers/meta_service_group.gotests/server/apiv2/handlers/meta_service_group_test.gotests/server/apiv2/handlers/testutil.gotools/pd-ctl/pdctl/command/meta_service_group_command.go
There was a problem hiding this comment.
♻️ Duplicate comments (3)
tools/pd-ctl/pdctl/command/meta_service_group_command.go (2)
121-125:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate that group IDs are non-empty.
The command trims each ID argument but doesn't validate it's non-empty. A user could accidentally run
delete ""or pass an argument that becomes empty after trimming, sending an invalid patch to the server.🛡️ Proposed fix to add validation
patch := make(map[string]*string) for _, id := range args { - patch[strings.TrimSpace(id)] = nil + trimmedID := strings.TrimSpace(id) + if trimmedID == "" { + cmd.PrintErrf("Invalid ID: cannot be empty or whitespace-only") + return + } + patch[trimmedID] = nil }🤖 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 `@tools/pd-ctl/pdctl/command/meta_service_group_command.go` around lines 121 - 125, In newDeleteMetaServiceGroupFunc validate each argument after strings.TrimSpace before adding to the patch map: if the trimmed id is empty, return or print a user-facing error and abort the command (don't add an empty key to patch). Ensure you reference the patch variable and args slice (and the loop over id) when implementing the check so any argument that becomes empty after trimming is rejected rather than sent to the server.
87-96:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate that group IDs and addresses are non-empty.
The command parses
--group id=addrflags but doesn't validate that the ID or address are non-empty after trimming. Users could accidentally specify--group id=(empty address) or--group =addr(empty ID), sending invalid patch requests to the server.🛡️ Proposed fix to add validation
for _, group := range metaServiceGroups { parts := strings.SplitN(group, "=", 2) if len(parts) != 2 { cmd.PrintErrf("Invalid --group format: %q (expected id=addr1,addr2,...)", group) return } + id := strings.TrimSpace(parts[0]) + if id == "" { + cmd.PrintErrf("Invalid --group: ID cannot be empty in %q", group) + return + } addr := strings.TrimSpace(parts[1]) + if addr == "" { + cmd.PrintErrf("Invalid --group: address cannot be empty in %q", group) + return + } - patch[strings.TrimSpace(parts[0])] = &addr + patch[id] = &addr }🤖 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 `@tools/pd-ctl/pdctl/command/meta_service_group_command.go` around lines 87 - 96, Trim and validate both the group ID and address before adding to the patch: after splitting the input in the loop over metaServiceGroups (variable parts), call strings.TrimSpace on parts[0] and parts[1], ensure neither trimmed ID nor trimmed addr is empty, and if either is empty use cmd.PrintErrf to report a clear error like "Invalid --group format: %q (id and addr must be non-empty)" and return; only then assign patch[trimmedID] = &addr. This change touches the loop that builds patch from metaServiceGroups and uses variables parts, addr, patch, and cmd.PrintErrf.server/apiv2/handlers/meta_service_group.go (1)
71-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate that group IDs and addresses are non-empty before applying the patch.
The handler accepts patch keys (group IDs) and values (addresses) without validating they are non-empty or non-whitespace. This allows clients to create or update meta-service groups with empty IDs (
{"": "address"}) or empty addresses ({"group-1": ""}), corrupting the configuration and potentially breaking keyspace assignment logic.🛡️ Proposed fix to add validation
+ for id := range patch { + if strings.TrimSpace(id) == "" { + c.AbortWithStatusJSON(http.StatusBadRequest, "group ID cannot be empty or whitespace-only") + return + } + } + currentGroups := manager.GetGroups() newGroups := make(map[string]string) maps.Copy(newGroups, currentGroups) for id, addresses := range patch { if addresses == nil { // Remove operation delete(newGroups, id) } else { // Add or update operation + if strings.TrimSpace(*addresses) == "" { + c.AbortWithStatusJSON(http.StatusBadRequest, "group addresses cannot be empty or whitespace-only") + return + } newGroups[id] = *addresses } }Don't forget to add
"strings"to the imports.🤖 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 `@server/apiv2/handlers/meta_service_group.go` around lines 71 - 89, The patch loop currently accepts empty or whitespace-only group IDs and addresses; validate inputs before mutating newGroups by trimming whitespace and rejecting any key where strings.TrimSpace(id)=="" or, for non-nil values, strings.TrimSpace(*addresses)=="" (preserve the existing delete behavior when addresses==nil). On validation failure return c.AbortWithStatusJSON(http.StatusBadRequest, errs.ErrBindJSON.Wrap(err).GenWithStackByCause()) or an appropriate bad-request error, and ensure you import "strings". Apply these checks in the handler around the patch iteration that manipulates patch, newGroups, manager.GetGroups(), and currentGroups.
🧹 Nitpick comments (1)
tests/server/apiv2/handlers/testutil.go (1)
346-360: ⚡ Quick winConsider renaming
mustAddMetaServiceGroupsto reflect PATCH semantics.The function name
mustAddMetaServiceGroupsno longer accurately describes its behavior since it now sends a PATCH request that can add, update, or delete groups based on the patch payload. Consider renaming to something likemustPatchMetaServiceGroupsormustApplyMetaServiceGroupPatchto better reflect the actual operation.♻️ Proposed refactor
-func mustAddMetaServiceGroups(re *require.Assertions, server *tests.TestServer, patch map[string]*string) []*handlers.MetaServiceGroupStatus { +func mustPatchMetaServiceGroups(re *require.Assertions, server *tests.TestServer, patch map[string]*string) []*handlers.MetaServiceGroupStatus {Then update all call sites in test files to use the new name.
🤖 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 `@tests/server/apiv2/handlers/testutil.go` around lines 346 - 360, Rename the function mustAddMetaServiceGroups to a name that reflects PATCH semantics (e.g., mustPatchMetaServiceGroups or mustApplyMetaServiceGroupPatch) and update all test call sites to use the new name; locate the function declaration (mustAddMetaServiceGroups) in testutil and change its identifier, then run a project-wide rename for any references in tests so compilation and imports remain consistent.
🤖 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.
Duplicate comments:
In `@server/apiv2/handlers/meta_service_group.go`:
- Around line 71-89: The patch loop currently accepts empty or whitespace-only
group IDs and addresses; validate inputs before mutating newGroups by trimming
whitespace and rejecting any key where strings.TrimSpace(id)=="" or, for non-nil
values, strings.TrimSpace(*addresses)=="" (preserve the existing delete behavior
when addresses==nil). On validation failure return
c.AbortWithStatusJSON(http.StatusBadRequest,
errs.ErrBindJSON.Wrap(err).GenWithStackByCause()) or an appropriate bad-request
error, and ensure you import "strings". Apply these checks in the handler around
the patch iteration that manipulates patch, newGroups, manager.GetGroups(), and
currentGroups.
In `@tools/pd-ctl/pdctl/command/meta_service_group_command.go`:
- Around line 121-125: In newDeleteMetaServiceGroupFunc validate each argument
after strings.TrimSpace before adding to the patch map: if the trimmed id is
empty, return or print a user-facing error and abort the command (don't add an
empty key to patch). Ensure you reference the patch variable and args slice (and
the loop over id) when implementing the check so any argument that becomes empty
after trimming is rejected rather than sent to the server.
- Around line 87-96: Trim and validate both the group ID and address before
adding to the patch: after splitting the input in the loop over
metaServiceGroups (variable parts), call strings.TrimSpace on parts[0] and
parts[1], ensure neither trimmed ID nor trimmed addr is empty, and if either is
empty use cmd.PrintErrf to report a clear error like "Invalid --group format: %q
(id and addr must be non-empty)" and return; only then assign patch[trimmedID] =
&addr. This change touches the loop that builds patch from metaServiceGroups and
uses variables parts, addr, patch, and cmd.PrintErrf.
---
Nitpick comments:
In `@tests/server/apiv2/handlers/testutil.go`:
- Around line 346-360: Rename the function mustAddMetaServiceGroups to a name
that reflects PATCH semantics (e.g., mustPatchMetaServiceGroups or
mustApplyMetaServiceGroupPatch) and update all test call sites to use the new
name; locate the function declaration (mustAddMetaServiceGroups) in testutil and
change its identifier, then run a project-wide rename for any references in
tests so compilation and imports remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 055a1df7-a77e-4b32-8cd4-13366a467680
📒 Files selected for processing (4)
server/apiv2/handlers/meta_service_group.gotests/server/apiv2/handlers/meta_service_group_test.gotests/server/apiv2/handlers/testutil.gotools/pd-ctl/pdctl/command/meta_service_group_command.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tools/pd-ctl/pdctl/command/meta_service_group_command.go (2)
121-125:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate that group IDs are non-empty.
The command trims each ID argument but doesn't validate it's non-empty. A user could accidentally run
delete ""or pass an argument that becomes empty after trimming, sending an invalid patch to the server.🛡️ Proposed fix to add validation
patch := make(map[string]*string) for _, id := range args { - patch[strings.TrimSpace(id)] = nil + trimmedID := strings.TrimSpace(id) + if trimmedID == "" { + cmd.PrintErrf("Invalid ID: cannot be empty or whitespace-only") + return + } + patch[trimmedID] = nil }🤖 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 `@tools/pd-ctl/pdctl/command/meta_service_group_command.go` around lines 121 - 125, In newDeleteMetaServiceGroupFunc, after trimming each id (currently assigned to patch with strings.TrimSpace(id)), validate that the trimmed value is not empty; if it is empty, print a clear error via the cobra command (e.g., cmd.PrintErrf or fmt.Fprintf(cmd.ErrOrStderr(), ...)) and return/exit with a non-zero status instead of adding an empty key to the patch map, ensuring no empty group IDs are sent to the server.
87-96:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate that group IDs and addresses are non-empty.
The command parses
--group id=addrflags but doesn't validate that the ID or address are non-empty after trimming. Users could accidentally specify--group id=(empty address) or--group =addr(empty ID), sending invalid patch requests to the server.🛡️ Proposed fix to add validation
for _, group := range metaServiceGroups { parts := strings.SplitN(group, "=", 2) if len(parts) != 2 { cmd.PrintErrf("Invalid --group format: %q (expected id=addr1,addr2,...)", group) return } + id := strings.TrimSpace(parts[0]) + if id == "" { + cmd.PrintErrf("Invalid --group: ID cannot be empty in %q", group) + return + } addr := strings.TrimSpace(parts[1]) + if addr == "" { + cmd.PrintErrf("Invalid --group: address cannot be empty in %q", group) + return + } - patch[strings.TrimSpace(parts[0])] = &addr + patch[id] = &addr }🤖 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 `@tools/pd-ctl/pdctl/command/meta_service_group_command.go` around lines 87 - 96, The loop building patch from metaServiceGroups must validate both the trimmed group ID and the trimmed address string are non-empty before inserting into patch; update the loop that splits group (strings.SplitN(...)) to trim parts[0] and parts[1], check that id != "" and addr != "" (and for addr, if comma-separated ensure none are empty segments), and if validation fails call cmd.PrintErrf with a clear "Invalid --group format or empty id/address: %q" message and return instead of adding to patch.
🤖 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 `@tools/pd-ctl/pdctl/command/meta_service_group_command.go`:
- Line 67: The command's Use string is misleading because the implementation
(the RunE handler that accepts _ []string and the required --group flag) expects
flag-based input; update the Use field on the upset command (in
meta_service_group_command.go where the command is constructed) to remove
positional-argument syntax and reflect a flag-only interface (e.g., change the
Use value to "upset" or "upset [flags]") so help output matches the required
--group flag; leave the delete command unchanged.
---
Duplicate comments:
In `@tools/pd-ctl/pdctl/command/meta_service_group_command.go`:
- Around line 121-125: In newDeleteMetaServiceGroupFunc, after trimming each id
(currently assigned to patch with strings.TrimSpace(id)), validate that the
trimmed value is not empty; if it is empty, print a clear error via the cobra
command (e.g., cmd.PrintErrf or fmt.Fprintf(cmd.ErrOrStderr(), ...)) and
return/exit with a non-zero status instead of adding an empty key to the patch
map, ensuring no empty group IDs are sent to the server.
- Around line 87-96: The loop building patch from metaServiceGroups must
validate both the trimmed group ID and the trimmed address string are non-empty
before inserting into patch; update the loop that splits group
(strings.SplitN(...)) to trim parts[0] and parts[1], check that id != "" and
addr != "" (and for addr, if comma-separated ensure none are empty segments),
and if validation fails call cmd.PrintErrf with a clear "Invalid --group format
or empty id/address: %q" message and return instead of adding to patch.
🪄 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: f7001277-6649-451f-ad38-3f58f41295c3
📒 Files selected for processing (2)
server/apiv2/handlers/meta_service_group.gotools/pd-ctl/pdctl/command/meta_service_group_command.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/apiv2/handlers/meta_service_group.go
- Validate non-empty group ID and addresses in PATCH handler - Reject deleting a group that still has assigned keyspaces - Fix misleading Use field on upset subcommand - Validate non-empty ID/address in upset and delete CLI commands - Add CLI integration tests for list/upset/delete and invalid inputs Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
ti-chi-bot
Bot
added
size/XL
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
ti-chi-bot
Bot
added
size/XXL
…lete rule Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/server/apiv2/handlers/meta_service_group_test.go`:
- Around line 166-173: The test deletes "etcd-group-4" while earlier setup
assigns keyspaces to newly added groups, violating the rule that groups with
assigned keyspaces cannot be deleted; update the test so the deletion target is
a group with no assigned keyspaces by either creating an extra unused group (via
mustAddMetaServiceGroups) specifically for deletion or selecting an existing
group that was never assigned keyspaces, and then assert it is removed; ensure
references to "etcd-group-4" in the deletePatch and subsequent assertions
reflect that unused group (or change the assertion to expect a delete failure if
you intend to test deletion-rejection for assigned groups).
🪄 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: 7a9e0f65-ea20-4444-bd76-23ae13a2bf85
📒 Files selected for processing (2)
tests/server/apiv2/handlers/meta_service_group_test.gotests/server/apiv2/handlers/testutil.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/server/apiv2/handlers/testutil.go
ti-chi-bot
Bot
added
size/XL
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10881 +/- ##
==========================================
+ Coverage 79.14% 79.18% +0.03%
==========================================
Files 536 541 +5
Lines 74033 75310 +1277
==========================================
+ Hits 58594 59634 +1040
- Misses 11311 11446 +135
- Partials 4128 4230 +102
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
bufferflies
force-pushed
the
pr-cp-10880-meta-service-group-patch
branch
from
31b8066 to
1e1fe2a
Compare
| for _, id := range groupIDs { | ||
| set[id] = struct{}{} | ||
| } | ||
| return m.keyspaceAssignmentCounter(set) |
There was a problem hiding this comment.
This scan misses keyspaces that PickGroup has assigned but CreateKeyspace has not saved yet. A concurrent delete can remove the group in that window and leave the new keyspace pointing to a deleted group. Please keep assignment and metadata save under the same lock/transaction boundary.
There was a problem hiding this comment.
Fixed in 08ad3ee. Group selection and the keyspace metadata save now happen under the same mgm read lock via assignGroupAndSaveKeyspace (PickGroup is split into pickGroupLocked for the lock-held caller). A concurrent delete takes the write lock, so it is excluded from that window: either the delete runs first (and findMinMetaGroup will not choose the removed group) or the keyspace is persisted first (and the delete's scan sees it and is rejected). No more in-flight gap.
| } | ||
| } | ||
| } | ||
| if err := persist(); err != nil { |
There was a problem hiding this comment.
When this delete succeeds, please also clear the persisted assignment counter for each deleted group. Otherwise a stale positive counter will come back if the same group ID is re-added and can skew list output and PickGroup balancing.
There was a problem hiding this comment.
Done in d390d55: added MetaServiceGroupStorage.RemoveAssignmentCount, and persistGroupsLocked now clears the persisted counter for each deleted group once the deletion is persisted (under the write lock). Re-adding a group with the same ID no longer inherits the stale count, so list output and PickGroup balancing stay correct.
After UpdateGroupsSafely removed a group, its persisted assignment counter key was left behind. Re-adding a group with the same ID would then inherit the stale count, skewing list output and PickGroup load balancing. Add MetaServiceGroupStorage.RemoveAssignmentCount and clear the counter for each deleted group once the deletion is persisted (best-effort under the write lock; the delete guard relies on actual keyspace scans, not this counter). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
|
/retest |
| if err := manager.mgm.updateAssignmentTxn(txn, oldMetaServiceGroup, newMetaServiceGroup); err != nil { | ||
| // The read lock held by runTxnWithMetaGroupLock keeps this validation and | ||
| // the assignment update atomic with respect to UpdateGroupsSafely. | ||
| if err := manager.mgm.reassignKeyspaceLocked(txn, oldMetaServiceGroup, newMetaServiceGroup); err != nil { |
There was a problem hiding this comment.
This path can leave state inconsistent. If the request changes both tso_keyspace_group_id and meta_service_group_id, UpdateKeyspaceGroup above has already persisted the TSO keyspace-group move before this validation can fail. When reassignKeyspaceLocked returns an error, the keyspace meta is not saved, but the keyspace-group membership has already changed. Please validate/reassign the meta-service group before moving the TSO group, or roll back the TSO group move on any later error.
There was a problem hiding this comment.
Addressed in 47da273: the order is now meta-service reassign/validation first, TSO keyspace-group move second.
reassignKeyspaceLockedruns beforeUpdateKeyspaceGroup(keyspace.go ~889-902). It only stages its changes in the txn, so if the meta-service validation/reassign fails, we return before the TSO group is ever moved.- The only step after the TSO move is
SaveKeyspaceMeta, and on its failure we roll the TSO group move back (keyspace.go ~914-919).
So a failing meta-service reassignment no longer leaves the TSO keyspace-group move persisted but unreverted.
|
/hold |
ti-chi-bot
Bot
added
the
do-not-merge/hold
In UpdateKeyspaceConfig, UpdateKeyspaceGroup persists the TSO keyspace-group move immediately, while reassignKeyspaceLocked only stages its writes in the txn. When a request changed both tso_keyspace_group_id and meta_service_group_id and the meta-service reassignment failed, the TSO group move was already persisted but never reverted, leaving inconsistent state. Do the fallible meta-service validation/reassign first so the immediate TSO group move is only performed once the meta-service change has succeeded. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
| if err != nil { | ||
| // Assign a meta-service group (if any exist) and save the keyspace atomically | ||
| // with respect to group deletion. The assignment is reflected in request.Config. | ||
| assignToMetaServiceGroup := manager.mgm != nil && len(manager.mgm.GetGroups()) > 0 |
There was a problem hiding this comment.
This pre-lock group check can become stale if a concurrent PATCH deletes the last group, causing keyspace creation to fail unnecessarily.
There was a problem hiding this comment.
Fixed in 6378652. The pre-lock assignToMetaServiceGroup check is now re-validated under the mgm read lock inside assignGroupAndSaveKeyspace: if a concurrent PATCH deleted the last group in the meantime (hasGroupsLocked() returns false), the keyspace is created without a meta-service group assignment instead of failing with "no available meta-service groups". Added a unit test (TestAssignGroupAndSaveKeyspace) covering both the stale-precheck path and normal assignment.
assignToMetaServiceGroup is computed before acquiring the mgm read lock, so a concurrent PATCH deleting the last group could make assignGroupAndSaveKeyspace fail with "no available meta-service groups" even though creating the keyspace without an assignment is the correct outcome. Re-check group availability under the read lock (hasGroupsLocked); if none remain, create the keyspace without a meta-service group instead of failing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
|
/retest |
|
/hold cancel |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
|
/retest |
1 similar comment
|
/retest |
|
@bufferflies: You cannot manually add or delete the reviewing state labels, only I and the tursted members have permission to do so. DetailsIn response to adding label named approved. 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 ti-community-infra/tichi repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AmoebaProtozoa, lhy1024, niubell, rleungx 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 |
|
/retest |
1 similar comment
|
/retest |
6 participants
What problem does this PR solve?
Issue Number: Close #10880
CP from 0b70d5d (original PR #380).
What is changed and how does it work?
Check List
Tests
Code changes
Related changes
Validation:
GOFLAGS=-buildvcs=false go test ./tests/server/apiv2/handlers -run TestMetaServiceGroupTestSuite -count=1 -timeout=5mcd tools/pd-ctl && GOFLAGS=-buildvcs=false go test ./pdctl/command -run 'Test.*MetaServiceGroup|TestNonExistent' -count=1 -timeout=5mgit diff --checkRelease note
Summary by CodeRabbit
New Features
PATCHwith JSON merge-patch semantics for both updates and deletions.CLI Updates
meta-service-groupnow providesupsertanddeletecommands (replacing the prioraddflow), including improved handling of batch operations and invalid inputs.Tests
PATCH-based behavior.