keyspace: add update keyspace config#69020
Conversation
Signed-off-by: ystaticy <y_static_y@sina.com>
ti-chi-bot
Bot
added
do-not-merge/invalid-title
do-not-merge/needs-linked-issue
do-not-merge/needs-tests-checked
release-note-none
|
@ystaticy I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
the
size/L
📝 WalkthroughWalkthroughThis PR adds exported ChangesKeyspace Configuration Feature
Sequence DiagramsequenceDiagram
participant Caller
participant InfoSyncer
participant pdHTTPCli
participant PD
Caller->>InfoSyncer: SetKeyspaceConfig(ctx, keyspaceName, config)
InfoSyncer->>pdHTTPCli: UpdateKeyspaceConfig(params)
pdHTTPCli->>PD: HTTP PATCH /keyspace/{name} with params
PD-->>pdHTTPCli: response (KeyspaceMeta or error)
pdHTTPCli-->>InfoSyncer: result / error
InfoSyncer-->>Caller: return error or nil
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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 |
|
Hi @ystaticy. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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. |
ystaticy
changed the title
ti-chi-bot
Bot
removed
do-not-merge/invalid-title
do-not-merge/needs-tests-checked
labels
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/domain/infosync/info_test.go`:
- Around line 279-343: Add a regression test in TestSetKeyspaceConfig that calls
SetKeyspaceConfig with a typed-nil params pointer to ensure the function
validates and returns a local error: after the existing happy-path case, invoke
SetKeyspaceConfig(context.Background(), "test-keyspace",
(*pdhttp.UpdateKeyspaceConfigParams)(nil)), assert require.Error(t, err) and
assert the error indicates invalid/nil params (e.g., require.Contains(t,
err.Error(), "params" or "nil")). This pins the conversion guard in
SetKeyspaceConfig and prevents regressions; reference SetKeyspaceConfig and
pdhttp.UpdateKeyspaceConfigParams when adding the assertion.
In `@pkg/domain/infosync/info.go`:
- Around line 359-365: The toUpdateKeyspaceConfigParams function currently
treats a typed nil *pdhttp.UpdateKeyspaceConfigParams as valid and returns (nil,
nil), allowing nil payloads to reach UpdateKeyspaceConfig; modify
toUpdateKeyspaceConfigParams to explicitly guard against typed-nil pointers:
when the config asserts to (*pdhttp.UpdateKeyspaceConfigParams) check if params
== nil and return a descriptive error (e.g., "nil UpdateKeyspaceConfigParams")
instead of (nil, nil); keep the existing handling for the non-pointer value case
(pdhttp.UpdateKeyspaceConfigParams) unchanged.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fec23760-9c38-4df1-b263-702655a0a48f
📒 Files selected for processing (3)
pkg/domain/infosync/BUILD.bazelpkg/domain/infosync/info.gopkg/domain/infosync/info_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69020 +/- ##
================================================
+ Coverage 76.3213% 76.3385% +0.0171%
================================================
Files 2041 2059 +18
Lines 562689 582171 +19482
================================================
+ Hits 429452 444421 +14969
- Misses 132324 135148 +2824
- Partials 913 2602 +1689
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
One concern about Could we reject this locally, e.g. return an error when |
Signed-off-by: ystaticy <y_static_y@sina.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/domain/infosync/info.go (1)
342-353:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffValidate that
config.Configis non-nil and non-empty before issuing the PD request.The function accepts
pdhttp.UpdateKeyspaceConfigParamsby value, which can be a zero-value struct whereConfigisnil. According to the PR objectives, when the PD backend receives a request withConfig == nilor an empty map, it interprets the payload as empty mutations and returns success without performing any configuration update. This creates a silent failure where callers may believe a keyspace configuration was successfully updated when the operation actually did nothing.Add local validation to reject:
- Cases where
config.Configisnil- Cases where
config.Configis an empty map🛡️ Proposed validation guard
func SetKeyspaceConfig(ctx context.Context, keyspaceName string, config pdhttp.UpdateKeyspaceConfigParams) error { is, err := getGlobalInfoSyncer() if err != nil { return errors.Trace(err) } if is.pdHTTPCli == nil { return errs.ErrClientGetLeader.FastGenByArgs("pd http cli is nil") } + if config.Config == nil || len(config.Config) == 0 { + return errors.New("keyspace config cannot be nil or empty") + } _, err = is.pdHTTPCli.UpdateKeyspaceConfig(ctx, keyspaceName, &config) return errors.Trace(err) }🤖 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/domain/infosync/info.go` around lines 342 - 353, The SetKeyspaceConfig function accepts a potentially zero-value UpdateKeyspaceConfigParams struct where the Config field can be nil or empty, and the PD backend will silently accept such requests without performing any actual configuration updates. Add validation in SetKeyspaceConfig before calling is.pdHTTPCli.UpdateKeyspaceConfig to check that config.Config is neither nil nor an empty map, returning an appropriate error if either condition is true. Place this validation after the pdHTTPCli nil check and before the UpdateKeyspaceConfig call to prevent silent failures where callers believe the configuration was updated when it was not.pkg/domain/infosync/info_test.go (1)
279-340:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd regression tests for nil and empty
Configmap validation.The test suite does not cover the validation gap where
config.Configisnilor empty. According to the PR objectives, these cases should be rejected locally with an explicit error rather than allowing the request to proceed to PD (which would silently succeed without updating anything).Please add test cases that:
- Pass
pdhttp.UpdateKeyspaceConfigParams{}(zero-value withConfig == nil) with a valid PD HTTP client and assert thatSetKeyspaceConfigreturns an error indicating invalid/nil config- Pass
pdhttp.UpdateKeyspaceConfigParams{Config: map[string]*string{}}(empty map) with a valid PD HTTP client and assert an errorThese tests will prevent future regressions and provide clear feedback to developers integrating this function.
📝 Example test structure
func TestSetKeyspaceConfigRejectsNilConfig(t *testing.T) { _, err := GlobalInfoSyncerInit(context.TODO(), "test", func() uint64 { return 1 }, nil, nil, nil, nil, keyspace.CodecV1, false, nil) require.NoError(t, err) restore := SetPDHttpCliForTest(&mockKeyspaceConfigPDHTTPClient{ t: t, expectedName: "test-keyspace", expectedParams: nil, // should not be called }) defer restore() // Test nil Config map err = SetKeyspaceConfig(context.Background(), "test-keyspace", pdhttp.UpdateKeyspaceConfigParams{}) require.Error(t, err) require.Contains(t, err.Error(), "config") // Test empty Config map err = SetKeyspaceConfig(context.Background(), "test-keyspace", pdhttp.UpdateKeyspaceConfigParams{ Config: map[string]*string{}, }) require.Error(t, err) require.Contains(t, err.Error(), "empty") }🤖 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/domain/infosync/info_test.go` around lines 279 - 340, Add two new regression test functions to validate that SetKeyspaceConfig rejects invalid Config maps. Following the same pattern as the existing test functions (TestSetKeyspaceConfig, TestSetKeyspaceConfigWithoutPDHTTPClient, TestSetKeyspaceConfigPropagatesPDHTTPError), create a test that initializes GlobalInfoSyncerInit with a valid mock PD HTTP client via SetPDHttpCliForTest, then calls SetKeyspaceConfig twice: once with a zero-value UpdateKeyspaceConfigParams (nil Config) and once with an empty Config map (map[string]*string{}). Both calls should return errors with appropriate messages indicating invalid or nil/empty config. This validates that SetKeyspaceConfig performs local validation before attempting to contact PD.
🤖 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.
Outside diff comments:
In `@pkg/domain/infosync/info_test.go`:
- Around line 279-340: Add two new regression test functions to validate that
SetKeyspaceConfig rejects invalid Config maps. Following the same pattern as the
existing test functions (TestSetKeyspaceConfig,
TestSetKeyspaceConfigWithoutPDHTTPClient,
TestSetKeyspaceConfigPropagatesPDHTTPError), create a test that initializes
GlobalInfoSyncerInit with a valid mock PD HTTP client via SetPDHttpCliForTest,
then calls SetKeyspaceConfig twice: once with a zero-value
UpdateKeyspaceConfigParams (nil Config) and once with an empty Config map
(map[string]*string{}). Both calls should return errors with appropriate
messages indicating invalid or nil/empty config. This validates that
SetKeyspaceConfig performs local validation before attempting to contact PD.
In `@pkg/domain/infosync/info.go`:
- Around line 342-353: The SetKeyspaceConfig function accepts a potentially
zero-value UpdateKeyspaceConfigParams struct where the Config field can be nil
or empty, and the PD backend will silently accept such requests without
performing any actual configuration updates. Add validation in SetKeyspaceConfig
before calling is.pdHTTPCli.UpdateKeyspaceConfig to check that config.Config is
neither nil nor an empty map, returning an appropriate error if either condition
is true. Place this validation after the pdHTTPCli nil check and before the
UpdateKeyspaceConfig call to prevent silent failures where callers believe the
configuration was updated when it was not.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7b1572c8-fb76-4bba-83c8-c0a1b98719b6
📒 Files selected for processing (2)
pkg/domain/infosync/info.gopkg/domain/infosync/info_test.go
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, YangKeao 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 |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
4 participants
What problem does this PR solve?
Issue Number: ref #67765
Problem Summary:
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests
Stability