meta-service-group: add status#10904
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
release-note-none
|
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:
📝 WalkthroughWalkthroughThe PR replaces per-group integer assignment-count persistence with JSON-persisted ChangesMeta-service group status migration
Sequence Diagram(s)sequenceDiagram
participant Client
participant PatchMetaServiceGroupStatus
participant MetaServiceGroupManager
participant StorageEndpoint
Client->>PatchMetaServiceGroupStatus: PATCH /meta-service-groups/:id/status
PatchMetaServiceGroupStatus->>MetaServiceGroupManager: PatchStatus(ctx, id, patch)
MetaServiceGroupManager->>StorageEndpoint: RunInTxn → LoadMetaServiceGroupStatus
StorageEndpoint-->>MetaServiceGroupManager: status map
MetaServiceGroupManager->>StorageEndpoint: SaveMetaServiceGroupStatus
PatchMetaServiceGroupStatus->>MetaServiceGroupManager: GetStatus(ctx)
MetaServiceGroupManager->>StorageEndpoint: LoadMetaServiceGroupStatus
StorageEndpoint-->>PatchMetaServiceGroupStatus: groups + statuses
PatchMetaServiceGroupStatus-->>Client: MetaServiceGroupStatus[]
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)
✨ 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/XL
|
@coderabbitai full review |
✅ 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
🤖 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 `@pkg/keyspace/meta_service_group.go`:
- Around line 75-79: The MetaServiceGroupStatusPatch struct uses the JSON field
name assigned_count for the AssignedCount field, but the MetaServiceGroupStatus
exposes this field as assignment_count. This mismatch prevents API clients from
updating the count using the status shape. Change the JSON tag on the
AssignedCount field in MetaServiceGroupStatusPatch to assignment_count to align
with the status struct. Additionally, add validation logic to reject negative
values before persisting the patch, since negative counts corrupt the balancing
logic. Be aware that callers constructing MetaServiceGroupStatusPatch instances
may need updating to use the corrected field name.
In `@pkg/storage/endpoint/meta_service_group.go`:
- Around line 62-70: In the loadMetaServiceGroupStatus function, when statusVal
is empty (indicating the /status key is missing), add a fallback mechanism
before returning the zero-value status. Before the return statement in the
statusVal == "" condition, attempt to load the legacy assignment-count key using
the transaction (txn). If the legacy key exists, migrate its integer value to
populate the assignment_count field of the status object and set enabled to
true. Only return the zero-value status with assignment_count:0 and
enabled:false if the legacy key is also not found, ensuring existing persisted
counts from the old storage model are preserved during the upgrade.
In `@server/apiv2/handlers/meta_service_group.go`:
- Around line 187-195: The PatchMetaServiceGroupStatus handler needs to validate
the patch payload before persisting it and properly classify errors. After the
ShouldBindJSON check succeeds, add validation logic to ensure the patch data is
valid (such as checking that assigned_count is not negative), and return a 400
Bad Request if validation fails. Additionally, when the manager.PatchStatus call
fails, distinguish between client errors (invalid patch data) and server errors
instead of treating all failures as 500 Internal Server Error. Return 400 for
validation or client-side errors and 500 only for actual server-side failures.
🪄 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: c4f09485-f513-4fe7-917b-6ff801605390
📒 Files selected for processing (9)
pkg/keyspace/meta_service_group.gopkg/keyspace/meta_service_group_test.gopkg/storage/endpoint/meta_service_group.gopkg/storage/meta_service_group_test.gopkg/utils/keypath/absolute_key_path.goserver/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
🧹 Nitpick comments (3)
tests/server/apiv2/handlers/meta_service_group_test.go (1)
78-100: 💤 Low valuePotential logic discrepancy in
collectStatussettingEnabled=true.The
collectStatushelper function at line 89 setsEnabled: truefor all collected statuses. However, this represents the expected state after keyspaces have been created and assigned to enabled groups. This is correct in the context of the test since groups are enabled at lines 108-112 before keyspaces are created at line 114.The logic is correct but could be clearer. Consider adding a comment explaining that this helper assumes all collected groups have been enabled before assignment, to avoid confusion.
🤖 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` around lines 78 - 100, Add a clarifying comment in the collectStatus function above or near the line where Enabled is set to true (within the initialization of the handlers.MetaServiceGroupStatus struct) to explicitly document the assumption that this helper function expects all collected meta service groups to have been enabled before keyspaces are assigned to them. This will make the intent clearer to future readers and prevent confusion about why Enabled is always set to true.tools/pd-ctl/pdctl/command/meta_service_group_command.go (1)
166-172: 💤 Low valueConsider validating non-negative assignment count client-side.
strconv.Atoiaccepts negative integers. While the server should also validate this, adding a client-side check provides better UX by catching errors early with a clear message.Proposed fix
func newSetMetaServiceGroupAssignmentCountFunc(cmd *cobra.Command, args []string) { groupID := strings.TrimSpace(args[0]) assignmentCount, err := strconv.Atoi(args[1]) if err != nil { cmd.PrintErrln("Invalid value for assignment count, must be an integer:", err) return } + if assignmentCount < 0 { + cmd.PrintErrln("Invalid value for assignment count, must be non-negative") + return + } patch := &keyspace.MetaServiceGroupStatusPatch{🤖 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 166 - 172, After successfully parsing the assignment count with strconv.Atoi in the newSetMetaServiceGroupAssignmentCountFunc function, add a validation check to ensure assignmentCount is not negative. If assignmentCount is less than zero, use cmd.PrintErrln to display an error message indicating that the assignment count must be a non-negative integer, then return early. This provides better user experience by catching invalid input on the client side before sending it to the server.pkg/keyspace/meta_service_group.go (1)
149-161: ⚡ Quick winRedundant status load within the same transaction.
findMinMetaGroup(called at line 151) already loads the entire status map viaLoadMetaServiceGroupStatus. Lines 155-158 load the same data again within the same transaction. Consider refactoringfindMinMetaGroupto return both the selected group and the status map, allowing reuse here.♻️ Suggested refactor
-func (m *MetaServiceGroupManager) findMinMetaGroup(txn kv.Txn) (string, error) { +func (m *MetaServiceGroupManager) findMinMetaGroup(txn kv.Txn) (string, map[string]*endpoint.MetaServiceGroupStatus, error) { statusMap, err := m.store.LoadMetaServiceGroupStatus(txn, m.metaServiceGroups) if err != nil { - return "", err + return "", nil, err } minCount := math.MaxInt var assignedGroup string for currentGroup, status := range statusMap { if status.Enabled && status.AssignmentCount < minCount { minCount = status.AssignmentCount assignedGroup = currentGroup } } if assignedGroup == "" { - return "", errNoAvailableMetaServiceGroups + return "", nil, errNoAvailableMetaServiceGroups } - return assignedGroup, nil + return assignedGroup, statusMap, nil }Then in
AssignToGroup:- assignedGroup, err = m.findMinMetaGroup(txn) + var statusMap map[string]*endpoint.MetaServiceGroupStatus + assignedGroup, statusMap, err = m.findMinMetaGroup(txn) if err != nil { return err } - statusMap, err := m.store.LoadMetaServiceGroupStatus(txn, m.metaServiceGroups) - if err != nil { - return err - } status := statusMap[assignedGroup]🤖 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/keyspace/meta_service_group.go` around lines 149 - 161, The code is redundantly loading the status map twice within the same transaction: once inside findMinMetaGroup and again at lines 155-158. Refactor the findMinMetaGroup method to return both the assigned group and the status map it already loads internally. Then update the call to findMinMetaGroup in the transaction to receive both return values, and remove the redundant m.store.LoadMetaServiceGroupStatus call. Finally, use the status map returned from findMinMetaGroup instead of loading it again.
🤖 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 `@pkg/keyspace/meta_service_group.go`:
- Around line 173-178: The assignment count decrement for the old group ID in
the statusMap has no validation to prevent it from going negative. Before
decrementing status.AssignmentCount for the oldGroupID in the statusMap, add a
check to ensure the count is greater than 0 to prevent it from becoming
negative, which would corrupt load balancing behavior in findMinMetaGroup. Only
perform the decrement and subsequent SaveMetaServiceGroupStatus call if the
count is positive.
---
Nitpick comments:
In `@pkg/keyspace/meta_service_group.go`:
- Around line 149-161: The code is redundantly loading the status map twice
within the same transaction: once inside findMinMetaGroup and again at lines
155-158. Refactor the findMinMetaGroup method to return both the assigned group
and the status map it already loads internally. Then update the call to
findMinMetaGroup in the transaction to receive both return values, and remove
the redundant m.store.LoadMetaServiceGroupStatus call. Finally, use the status
map returned from findMinMetaGroup instead of loading it again.
In `@tests/server/apiv2/handlers/meta_service_group_test.go`:
- Around line 78-100: Add a clarifying comment in the collectStatus function
above or near the line where Enabled is set to true (within the initialization
of the handlers.MetaServiceGroupStatus struct) to explicitly document the
assumption that this helper function expects all collected meta service groups
to have been enabled before keyspaces are assigned to them. This will make the
intent clearer to future readers and prevent confusion about why Enabled is
always set to true.
In `@tools/pd-ctl/pdctl/command/meta_service_group_command.go`:
- Around line 166-172: After successfully parsing the assignment count with
strconv.Atoi in the newSetMetaServiceGroupAssignmentCountFunc function, add a
validation check to ensure assignmentCount is not negative. If assignmentCount
is less than zero, use cmd.PrintErrln to display an error message indicating
that the assignment count must be a non-negative integer, then return early.
This provides better user experience by catching invalid input on the client
side before sending it to the server.
🪄 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: beec8daa-b422-480f-885e-71d385de0445
📒 Files selected for processing (9)
pkg/keyspace/meta_service_group.gopkg/keyspace/meta_service_group_test.gopkg/storage/endpoint/meta_service_group.gopkg/storage/meta_service_group_test.gopkg/utils/keypath/absolute_key_path.goserver/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
| AssignedKeyspaces int `json:"assigned_keyspaces"` | ||
| ID string `json:"id"` | ||
| Addresses string `json:"addresses"` | ||
| Status *endpoint.MetaServiceGroupStatus `json:"status,omitempty"` |
There was a problem hiding this comment.
This changes the existing v2 response shape by removing assigned_keyspaces. Existing clients of GET/POST /meta-service-groups may break; please keep the old field while adding status, or version the API change.
There was a problem hiding this comment.
Good point. Restored the assigned_keyspaces field on the response in 10f7107 — it now mirrors status.assignment_count, so existing GET/POST /meta-service-groups clients keep working, while the new status object additionally exposes the enabled state. Happy to instead version the endpoint if you'd prefer a cleaner break.
ti-chi-bot
Bot
added
size/XXL
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 `@server/apiv2/handlers/meta_service_group.go`:
- Around line 161-169: The status assembly in MetaServiceGroupStatus is
returning a partial object when statuses[id] is missing, which can hide the
enabled state while still emitting AssignedKeyspaces as 0. Update the GetStatus
response handling in meta_service_group.go so it does not append a group unless
a matching endpoint.MetaServiceGroupStatus exists, or else build a full default
status consistently before appending. Keep the behavior aligned with the
MetaServiceGroupStatus struct fields and the statuses[id] lookup.
🪄 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: 5a34b952-41f3-4062-ba82-a462f7dd119a
📒 Files selected for processing (2)
pkg/keyspace/meta_service_group.goserver/apiv2/handlers/meta_service_group.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/keyspace/meta_service_group.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10904 +/- ##
==========================================
- Coverage 79.18% 79.16% -0.03%
==========================================
Files 541 541
Lines 75312 75603 +291
==========================================
+ Hits 59638 59849 +211
- Misses 11445 11519 +74
- Partials 4229 4235 +6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| c.AbortWithStatusJSON(http.StatusBadRequest, err.Error()) | ||
| return | ||
| } | ||
| c.AbortWithStatusJSON(http.StatusInternalServerError, err.Error()) |
There was a problem hiding this comment.
Please map errs.ErrEtcdTxnConflict to 409 here. Concurrent status patches or assignments can hit the etcd txn compare failure, and that is a retryable conflict rather than an internal server error.
There was a problem hiding this comment.
Fixed in 6f0571d: errs.ErrEtcdTxnConflict is now mapped to HTTP 409 on both the /status patch and the group patch endpoints, so a lost etcd txn compare surfaces as a retryable conflict rather than 500.
| if manager.mgm != nil && oldMetaServiceGroup != newMetaServiceGroup { | ||
| if newMetaServiceGroup != "" && manager.mgm.GetGroups()[newMetaServiceGroup] == "" { | ||
| return errUnknownMetaServiceGroup | ||
| return ErrUnknownMetaServiceGroup |
There was a problem hiding this comment.
This only checks that the target group exists. Since disabled groups are skipped by automatic assignment, config updates should also reject moving a keyspace into a disabled meta-service group.
There was a problem hiding this comment.
Fixed in 6f0571d: reassignKeyspaceLocked now loads the target group's status and rejects moving a keyspace into a disabled group with a new ErrMetaServiceGroupDisabled, mapped to HTTP 400 in the keyspace config-update handler.
10f7107 to
a9663e8
Compare
| if currentCount < minCount { | ||
| minCount = currentCount | ||
| for currentGroup, status := range statusMap { | ||
| if status.Enabled && status.AssignmentCount < minCount { |
There was a problem hiding this comment.
CI is failing because all configured groups start disabled, so keyspace creation gets no available meta-service groups. Please initialize existing groups as enabled or skip assignment when no enabled group exists.
There was a problem hiding this comment.
Fixed in 6f0571d: assignGroupAndSaveKeyspace now skips assignment and creates the keyspace without a meta-service group when no enabled group exists (instead of failing with no available meta-service groups), mirroring the empty-group-set fallback. Added a unit test covering the disabled-group case.
* add enable status Signed-off-by: AmoebaProtozoa <8039876+AmoebaProtozoa@users.noreply.github.com> * Update pkg/keyspace/meta_service_group.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update pkg/keyspace/meta_service_group.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update tests/server/apiv2/handlers/testutil.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix typo Signed-off-by: AmoebaProtozoa <8039876+AmoebaProtozoa@users.noreply.github.com> * fix linter issue Signed-off-by: AmoebaProtozoa <8039876+AmoebaProtozoa@users.noreply.github.com> * Update tools/pd-ctl/pdctl/command/meta_service_group_command.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix copilot suggestion Signed-off-by: AmoebaProtozoa <8039876+AmoebaProtozoa@users.noreply.github.com> * fix ut Signed-off-by: AmoebaProtozoa <8039876+AmoebaProtozoa@users.noreply.github.com> --------- Signed-off-by: AmoebaProtozoa <8039876+AmoebaProtozoa@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> (cherry picked from commit 5be7eae) Signed-off-by: bufferflies <1045931706@qq.com>
a9663e8 to
529cb1c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/storage/meta_service_group_test.go (1)
68-124: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a delete round-trip to the storage suite.
This covers save/load and missing-key defaults, but it never verifies that
RemoveMetaServiceGroupStatusactually clears persisted state. That is part of the changed contract, and without a direct test a deleted group could silently inherit staleenabled/assignment_countif the same ID is added again.Proposed test addition
checkStatus(re, store, result) + re.NoError(store.RunInTxn(context.TODO(), func(txn kv.Txn) error { + return store.RemoveMetaServiceGroupStatus(txn, "group3") + })) + checkStatus(re, store, map[string]*endpoint.MetaServiceGroupStatus{ + "group3": {}, + }) // should treat extra groups as having empty disabled status. extraGroups := map[string]*endpoint.MetaServiceGroupStatus{ "group4": {}, "group5": {}, }🤖 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/meta_service_group_test.go` around lines 68 - 124, The storage test in TestMetaServiceGroupStorage should also cover the delete round-trip for MetaServiceGroupStatus. After exercising save/load with existing cases, add a scenario that calls RemoveMetaServiceGroupStatus on a known group via the storage API, then verify the status is cleared and later re-adding the same group does not inherit stale AssignmentCount or Enabled values. Use the existing checkStatus helper and the MetaServiceGroupStatus storage methods to locate the right behavior.tools/pd-ctl/tests/metaservicegroup/meta_service_group_test.go (1)
116-142: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that rejected patch commands leave status unchanged.
These branches only check the error text. If a bad request partially mutated persisted status before failing, this test would still pass. Re-list after each rejected command and confirm
etcd-group-0is stillenabled=falsewithassignment_count=10.🤖 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/tests/metaservicegroup/meta_service_group_test.go` around lines 116 - 142, The rejected patch-command checks in meta-service-group tests only verify error messages and do not confirm state was unchanged after failure. In the test cases around ctl.GetRootCmd() and tests.MustExec for set-enabled and set-assignment-count, re-list the target group after each rejected command and assert the persisted status for etcd-group-0 still shows enabled=false and assignment_count=10. Keep the checks alongside the existing error assertions so any partial mutation in the patch flow is caught.
🤖 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 `@pkg/keyspace/meta_service_group.go`:
- Around line 190-196: In AssignToGroup, reject negative assignment increments
before updating and saving the MetaServiceGroup status; the current
status.AssignmentCount += count path can persist invalid negative counts and
skew findMinMetaGroup. Add an early validation check in AssignToGroup, before
LoadMetaServiceGroupStatus/SaveMetaServiceGroupStatus, so only non-negative
counts are applied.
In `@server/apiv2/handlers/meta_service_group.go`:
- Around line 173-183: The Swagger annotation for PatchMetaServiceGroupStatus is
missing the path parameter for the {id} segment in the
/meta-service-groups/{id}/status route. Update the comment block on
PatchMetaServiceGroupStatus to declare the id path param alongside the existing
body param, then regenerate the spec with make swagger-spec (SWAGGER=1) so the
generated OpenAPI stays in sync.
---
Nitpick comments:
In `@pkg/storage/meta_service_group_test.go`:
- Around line 68-124: The storage test in TestMetaServiceGroupStorage should
also cover the delete round-trip for MetaServiceGroupStatus. After exercising
save/load with existing cases, add a scenario that calls
RemoveMetaServiceGroupStatus on a known group via the storage API, then verify
the status is cleared and later re-adding the same group does not inherit stale
AssignmentCount or Enabled values. Use the existing checkStatus helper and the
MetaServiceGroupStatus storage methods to locate the right behavior.
In `@tools/pd-ctl/tests/metaservicegroup/meta_service_group_test.go`:
- Around line 116-142: The rejected patch-command checks in meta-service-group
tests only verify error messages and do not confirm state was unchanged after
failure. In the test cases around ctl.GetRootCmd() and tests.MustExec for
set-enabled and set-assignment-count, re-list the target group after each
rejected command and assert the persisted status for etcd-group-0 still shows
enabled=false and assignment_count=10. Keep the checks alongside the existing
error assertions so any partial mutation in the patch flow is caught.
🪄 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: 6a2dab0f-4f3a-4a9a-a426-961150b51ebd
📒 Files selected for processing (12)
pkg/keyspace/keyspace_test.gopkg/keyspace/meta_service_group.gopkg/keyspace/meta_service_group_test.gopkg/keyspace/util.gopkg/storage/endpoint/meta_service_group.gopkg/storage/meta_service_group_test.gopkg/utils/keypath/absolute_key_path.goserver/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.gotools/pd-ctl/tests/metaservicegroup/meta_service_group_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/server/apiv2/handlers/testutil.go
- pkg/keyspace/util.go
- tools/pd-ctl/pdctl/command/meta_service_group_command.go
- pkg/utils/keypath/absolute_key_path.go
- tests/server/apiv2/handlers/meta_service_group_test.go
- Skip meta-service group assignment when no enabled group exists instead
of failing keyspace creation, mirroring the empty-group-set fallback.
- Reject moving a keyspace into a disabled group on config update
(new ErrMetaServiceGroupDisabled, mapped to 400).
- Map errs.ErrEtcdTxnConflict to HTTP 409 on the meta-service group patch
endpoints, since a lost etcd txn compare is a retryable conflict.
- Reject negative count in AssignToGroup; synthesize a default status in
the list response so the enabled state is never dropped; add the missing
Swagger {id} path param.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: tongjian <1045931706@qq.com>
…eassign Cover AssignToGroup rejecting a negative count and reassignKeyspaceLocked rejecting a disabled or unknown target group. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
|
/retest |
| func (m *MetaServiceGroupManager) updateAssignmentTxn(txn kv.Txn, oldGroupID, newGroupID string) error { | ||
| if oldGroupID != "" { | ||
| if err := m.store.IncrementAssignmentCount(txn, oldGroupID, -1); err != nil { | ||
| statusMap, err := m.store.LoadMetaServiceGroupStatus(txn, m.metaServiceGroups) |
There was a problem hiding this comment.
This still reads m.metaServiceGroups, but callers like RemoveKeyspace reach updateAssignmentTxn without holding the meta-service group lock. Please load only oldGroupID/newGroupID or require the lock at every call site.
There was a problem hiding this comment.
Fixed in 82c8560: updateAssignmentTxn no longer reads the shared m.metaServiceGroups map. It now loads only the affected old/new group status by id (new loadGroupStatus helper), so unlocked callers like RemoveKeyspace no longer race with UpdateGroupsSafely. The old group's decrement is only persisted when it still has assignments, so a deleted group's status key isn't recreated. Verified with -race.
updateAssignmentTxn read the shared m.metaServiceGroups map, but callers like RemoveKeyspace reach it without holding the meta-service group lock, racing with UpdateGroupsSafely. Load only the affected old/new group status by id instead, and only persist the old group's decrement when it still has assignments so a deleted group's status is not recreated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
Group IDs could contain '/', so such a group could be created and listed
but never patched or enabled via /meta-service-groups/{id}/status. Restrict
IDs to URL-safe characters ([A-Za-z0-9_-]) in AdjustMetaServiceGroups and
reject them with 400 in the add/update handler.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: tongjian <1045931706@qq.com>
|
/test pull-unit-test-next-gen-3 |
|
/test pull-unit-test-next-gen-3 |
| // characters so that a group is always addressable via the | ||
| // /meta-service-groups/{id}/status endpoint (a '/' would otherwise split the | ||
| // path and make the group impossible to patch). | ||
| var metaServiceGroupIDPattern = regexp.MustCompile(`^[A-Za-z0-9_-]{1,64}$`) |
There was a problem hiding this comment.
This new validation can break existing configurations.
There was a problem hiding this comment.
Could you confirm whether all current meta-service group IDs already satisfy this new format? If yes, this should be fine; otherwise we may need to only reject /, or encode/escape the path parameter instead.
There was a problem hiding this comment.
Good question. The only IDs currently in use in tests/config are like group-0/etcd-group-0, which are fine either way. To be safe against existing IDs I relaxed the validation in 94c8a93 to reject only / (the character that would break the /{id}/status path); everything else (., :, etc.) is left untouched. This keeps @rleungx's addressability requirement while not risking any existing group ID.
| m.RLock() | ||
| defer m.RUnlock() | ||
| return m.store.RunInTxn(ctx, func(txn kv.Txn) error { | ||
| statusMap, err := m.store.LoadMetaServiceGroupStatus(txn, m.metaServiceGroups) |
There was a problem hiding this comment.
A small efficiency/conflict-scope question: when patching one group's status, this currently loads every group's status. With the etcd backend, all loaded status keys become transaction compare conditions, so patching group-a may return 409 if group-b is concurrently updated by keyspace assignment.
Would it be better to first check m.metaServiceGroups[groupID] under the lock to validate existence, then load only the target groupID's status? That would keep the read/write set smaller and avoid conflicts caused by unrelated group updates.
There was a problem hiding this comment.
Good point. Fixed in 9ee11c3: PatchStatus now validates m.metaServiceGroups[groupID] under the read lock and then loads only the target group's status in the txn (via loadGroupStatus), so the etcd compare set is limited to that one key and a concurrent assignment on another group no longer triggers a spurious 409.
| @@ -0,0 +1,143 @@ | |||
| // Copyright 2026 TiKV Project Authors. | |||
There was a problem hiding this comment.
This adds tools/pd-ctl/tests/metaservicegroup, but the repo already has tools/pd-ctl/tests/meta_service_group. Splitting tests for the same pd-ctl command across two differently named directories makes them easy to miss during future maintenance. Please move these new tests into the existing meta_service_group directory.
There was a problem hiding this comment.
Fixed in 9ee11c3: moved the set-enabled/set-assignment-count tests into the existing tools/pd-ctl/tests/meta_service_group suite (as TestSetEnabled/TestSetAssignmentCount/TestSetStatusInvalidInput) and removed the duplicate metaservicegroup directory.
… tests PatchStatus loaded every group's status, widening the etcd txn compare set so a patch could fail with a spurious conflict when an unrelated group was concurrently updated. Validate existence against the in-memory group set under the lock, then load only the target group's status in the txn. Also move the set-enabled/set-assignment-count pd-ctl tests into the existing tools/pd-ctl/tests/meta_service_group suite instead of a second directory with a near-duplicate name. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
| re.Contains(string(output), "address cannot contain empty segment") | ||
| } | ||
|
|
||
| func (suite *metaServiceGroupCLITestSuite) findGroup(groups []*handlers.MetaServiceGroupStatus, id string) *handlers.MetaServiceGroupStatus { |
There was a problem hiding this comment.
Fixed in 94c8a93: findGroup is now a free function instead of a suite method (it didn't use the receiver), which clears the unused-receiver lint.
| // characters so that a group is always addressable via the | ||
| // /meta-service-groups/{id}/status endpoint (a '/' would otherwise split the | ||
| // path and make the group impossible to patch). | ||
| var metaServiceGroupIDPattern = regexp.MustCompile(`^[A-Za-z0-9_-]{1,64}$`) |
There was a problem hiding this comment.
Could you confirm whether all current meta-service group IDs already satisfy this new format? If yes, this should be fine; otherwise we may need to only reject /, or encode/escape the path parameter instead.
Relax the group ID validation to reject only '/' (the character that would
make a group unpatchable via /meta-service-groups/{id}/status) instead of a
strict whitelist, so existing IDs containing '.', ':' etc. are not broken.
Also make the pd-ctl test findGroup a free function to satisfy the
unused-receiver linter.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: tongjian <1045931706@qq.com>
| cmd.PrintErrln("Failed to marshal request:", err) | ||
| return | ||
| } | ||
| resp, err := doRequest(cmd, metaServiceGroupPrefix+"/"+groupID+"/status", http.MethodPatch, |
There was a problem hiding this comment.
This still concatenates the raw group ID into the URL path. Since the server now only rejects /, IDs containing ?, #, %, or whitespace can be created via the JSON patch API but cannot be reliably enabled through pd-ctl because those characters are parsed as URL syntax before reaching the handler. The same applies to set-assignment-count below.
There was a problem hiding this comment.
Fixed in 5fcc61b: both set-enabled and set-assignment-count now wrap the group ID with url.PathEscape when building the request path, so ?, #, % and whitespace are percent-encoded instead of being parsed as URL syntax. gin decodes the :id path param on the server side, so the round trip is exact. Added a pd-ctl test that creates a group with a special-character ID (group a?b#c%d) and enables it through the CLI.
The server now only rejects '/', so a group ID may contain '?', '#', '%' or whitespace. pd-ctl concatenated the raw ID into the request path, where those characters are parsed as URL syntax and never reach the handler. Escape the ID with url.PathEscape (gin decodes the path param on the other side) so set-enabled/set-assignment-count work for any valid ID. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
/retest |
| if countVal == "" { | ||
| return 0, nil | ||
| status := &MetaServiceGroupStatus{} | ||
| if statusVal == "" { |
There was a problem hiding this comment.
On upgrade, existing clusters may only have the old /assignment_count key. Returning an empty disabled status here makes assigned_keyspaces drop to 0 and makes all existing groups ineligible for automatic assignment until status is manually patched, so the PR loses the persisted assignment state during the status-key migration.
| if id == "" { | ||
| return errors.New("[keyspace] meta-service group ID cannot be empty") | ||
| } | ||
| if !IsValidMetaServiceGroupID(id) { |
There was a problem hiding this comment.
Existing configs can already contain / in a meta-service group ID. Since this validation runs in the startup/config adjust path, such a cluster fails validation on upgrade before it can use the API delete escape hatch, so this is still a config compatibility break for legacy IDs.
|
/retest |
|
[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 |
5 participants
What problem does this PR solve?
Issue Number: Close #10889
CP from 5be7eae (original PR #392).
Original author: @AmoebaProtozoa.
What is changed and how does it work?
Check List
Tests
Code changes
Related changes
Validation:
GOFLAGS=-buildvcs=false go test ./pkg/keyspace ./pkg/storage ./server/apiv2/handlers -run 'TestMetaServiceGroup|TestNonExistent' -count=1 -timeout=5mGOFLAGS=-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 TestNonExistent -count=0 -timeout=5mgit diff --checkGOFLAGS=-buildvcs=false make checkRelease note
Summary by CodeRabbit
New Features
enabledandassignment_count, plus a companion CLI to set enabled state and assignment count.Bug Fixes
400responses for unknown/disabled groups, and keyspaces can be created when no groups are enabled.