client: remove prefix watch for resource group config by bufferflies · Pull Request #10920 · tikv/pd

bufferflies · 2026-06-22T12:05:59Z

What problem does this PR solve?

Issue Number: Close #10919

The resource group controller config watcher only needs to watch the controller config key, but it currently also passes opt.WithPrefix(). This makes the watch range broader than necessary.

What is changed and how does it work?

Remove `opt.WithPrefix()` from resource group controller config watch creation and retry paths.

Keep prefix watching unchanged for resource group metadata watches because those still need to watch metadata under a key prefix.

Check List

Tests

Manual test (code compiles through gofmt/diff check only; no full test run in this step)

Release note

None.

Summary by CodeRabbit

Bug Fixes
- Improved reliability of controller RU configuration updates by adjusting the controller metadata watch behavior so changes are consistently received and processed during startup and retry recovery.
Tests
- Updated the integration test to store controller configuration at the correct location and to preserve/restore any pre-existing value, preventing cross-test side effects while keeping watch-count and RU-version polling checks the same.

Signed-off-by: tongjian <1045931706@qq.com>

coderabbitai · 2026-06-22T12:06:16Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 66d4d0be-6a44-4e6a-82f3-8b2504ebfef8

📥 Commits

Reviewing files that changed from the base of the PR and between e90d177 and 198e17d.

📒 Files selected for processing (1)

tests/integrations/mcs/resourcemanager/resource_manager_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

tests/integrations/mcs/resourcemanager/resource_manager_test.go

📝 Walkthrough

Walkthrough

In ResourceGroupsController.Start, two provider.Watch calls for watchConfigChannel are changed to remove opt.WithPrefix(). Both the initial setup call (line 346) and the retry call (line 378) now pass only opt.WithRev(cfgRevision), narrowing the watch to the exact controller config key. The test is updated to preserve and restore existing config state, and to trigger the config watch by writing the marshaled config bytes directly to the prefix key instead of a child key.

Changes

Narrow controller config watch range

Layer / File(s)	Summary
Remove WithPrefix from config watch and update test `client/resource_group/controller/global_controller.go`, `tests/integrations/mcs/resourcemanager/resource_manager_test.go`	Both the initial and retry `provider.Watch` calls for `watchConfigChannel` drop `opt.WithPrefix()`, leaving only `opt.WithRev(cfgRevision)`. The watch now targets the exact controller config key instead of all keys sharing its prefix. The test is updated to preserve and restore the existing controller config before and after the test, and to trigger the config watch by writing the marshaled config bytes directly to the prefix key, aligning with the narrower watch behavior.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Suggested reviewers

rleungx
okJiang

Poem

🐇 A prefix too wide, a watcher astray,
Snipping the options, I hop on my way.
Just the one key now, no siblings to see,
The config watch lands where it's supposed to be.
snip snip — cleaner hops, from this PR's decree! 🌿

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: removing prefix watch for resource group config in the client controller.
Description check	✅ Passed	The PR description fully completes the required template with issue reference, clear problem statement, detailed change explanation, and appropriate release note.
Linked Issues check	✅ Passed	The PR fully addresses issue `#10919`: removes WithPrefix() from both config watch creation and retry paths, preserving prefix watches for metadata.
Out of Scope Changes check	✅ Passed	All changes are directly related to the scope defined in issue `#10919`; the test modification appropriately supports verification of the config watch behavior.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

Signed-off-by: tongjian <1045931706@qq.com>

…ies/pd into enhancement/remove_prefix_watch

The WatchCount test overwrites the shared controller config key with DefaultConfig, which pollutes other config-related tests after removing the prefix watch. Save the original config and restore it on cleanup. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>

coderabbitai

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/integrations/mcs/resourcemanager/resource_manager_test.go`:
- Around line 566-573: The defer cleanup function returns early when
originalConfig.GetKvs() is empty, but this fails to clean up if the key
pd.ControllerConfigPathPrefixBytes was created during the test, leaving
persistent state behind. Instead of returning early when
len(originalConfig.GetKvs()) equals zero, add a call to suite.client.Delete with
suite.ctx and pd.ControllerConfigPathPrefixBytes to properly remove the key that
was added during the test. Keep the existing Put logic for when the original
configuration did have 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: 5b4cd987-f229-46a4-afe8-2293a2a1506f

📥 Commits

Reviewing files that changed from the base of the PR and between 602f124 and e90d177.

📒 Files selected for processing (1)

tests/integrations/mcs/resourcemanager/resource_manager_test.go

bufferflies · 2026-06-22T13:14:10Z

/retest

bufferflies · 2026-06-23T02:11:19Z

/retest

codecov · 2026-06-23T02:18:51Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.17%. Comparing base (4ae1408) to head (198e17d).
⚠️ Report is 9 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10920      +/-   ##
==========================================
+ Coverage   79.15%   79.17%   +0.01%     
==========================================
  Files         540      540              
  Lines       74801    74801              
==========================================
+ Hits        59212    59223      +11     
+ Misses      11405    11397       -8     
+ Partials     4184     4181       -3

Flag	Coverage Δ
unittests	`79.17% <100.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ti-chi-bot · 2026-06-23T02:30:35Z

@YuhaoZhang00: adding LGTM is restricted to approvers and reviewers in OWNERS files.

Details

In response to this:

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 kubernetes-sigs/prow repository.

bufferflies · 2026-06-23T06:34:07Z

/retest

Signed-off-by: tongjian <1045931706@qq.com>

bufferflies · 2026-06-23T06:53:31Z

/retest

ti-chi-bot · 2026-06-23T08:55:43Z

[LGTM Timeline notifier]

Timeline:

2026-06-23 02:33:45.116237198 +0000 UTC m=+1308.779462710: ☑️ agreed by okJiang.
2026-06-23 08:55:42.561323087 +0000 UTC m=+24226.224548641: ☑️ agreed by rleungx.

bufferflies · 2026-06-23T09:00:51Z

/retest

ti-chi-bot · 2026-06-23T12:02:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lhy1024, okJiang, rleungx, YuhaoZhang00

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [lhy1024,okJiang,rleungx]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

bufferflies · 2026-06-24T13:36:24Z

/retest

bufferflies · 2026-06-25T02:23:32Z

/retest

bufferflies · 2026-06-25T02:29:29Z

/retest

bufferflies · 2026-06-25T02:39:32Z

/retest

bufferflies · 2026-06-25T02:51:16Z

/retest

bufferflies · 2026-06-25T03:01:27Z

/retest

bufferflies · 2026-06-25T03:11:45Z

/retest

bufferflies · 2026-06-25T03:32:16Z

/retest

client: remove prefix watch for resource group config

59bf47d

Signed-off-by: tongjian <1045931706@qq.com>

ti-chi-bot Bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has signed the dco. labels Jun 22, 2026

ti-chi-bot Bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 22, 2026

bufferflies and others added 3 commits June 22, 2026 20:23

client: remove prefix watch for resource group config

7117e78

Signed-off-by: tongjian <1045931706@qq.com>

Merge branch 'enhancement/remove_prefix_watch' of github.com:bufferfl…

602f124

…ies/pd into enhancement/remove_prefix_watch

ti-chi-bot Bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 22, 2026

coderabbitai Bot reviewed Jun 22, 2026

View reviewed changes

Comment thread tests/integrations/mcs/resourcemanager/resource_manager_test.go

okJiang mentioned this pull request Jun 22, 2026

Flaky: TestKeyspaceGroupTestSuite/TestUpdateMemberWhenRecovery times out with “failed to find the keyspace group” #10551

Open

YuhaoZhang00 approved these changes Jun 23, 2026

View reviewed changes

okJiang approved these changes Jun 23, 2026

View reviewed changes

ti-chi-bot Bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jun 23, 2026

lhy1024 reviewed Jun 23, 2026

View reviewed changes

Comment thread tests/integrations/mcs/resourcemanager/resource_manager_test.go

Comment thread tests/integrations/mcs/resourcemanager/resource_manager_test.go

address comment

198e17d

Signed-off-by: tongjian <1045931706@qq.com>

bufferflies requested a review from lhy1024 June 23, 2026 06:42

rleungx approved these changes Jun 23, 2026

View reviewed changes

ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jun 23, 2026

lhy1024 approved these changes Jun 23, 2026

View reviewed changes

ti-chi-bot Bot merged commit 2e12f48 into tikv:master Jun 25, 2026
41 of 43 checks passed

Uh oh!

Conversation

bufferflies commented Jun 22, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Suggested reviewers

Poem

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bufferflies commented Jun 22, 2026

Uh oh!

bufferflies commented Jun 23, 2026

Uh oh!

codecov Bot commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ti-chi-bot Bot commented Jun 23, 2026

Uh oh!

Uh oh!

Uh oh!

bufferflies commented Jun 23, 2026

Uh oh!

bufferflies commented Jun 23, 2026

Uh oh!

ti-chi-bot Bot commented Jun 23, 2026

[LGTM Timeline notifier]

Uh oh!

bufferflies commented Jun 23, 2026

Uh oh!

ti-chi-bot Bot commented Jun 23, 2026

Uh oh!

bufferflies commented Jun 24, 2026

Uh oh!

bufferflies commented Jun 25, 2026

Uh oh!

bufferflies commented Jun 25, 2026

Uh oh!

bufferflies commented Jun 25, 2026

Uh oh!

bufferflies commented Jun 25, 2026

Uh oh!

bufferflies commented Jun 25, 2026

Uh oh!

bufferflies commented Jun 25, 2026

Uh oh!

bufferflies commented Jun 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bufferflies commented Jun 22, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 22, 2026 •

edited

Loading

codecov Bot commented Jun 23, 2026 •

edited

Loading