http client: support update keyspce config#10717
Conversation
Signed-off-by: ystaticy <y_static_y@sina.com>
|
Hi @ystaticy. Thanks for your PR. I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds HTTP-client support to patch keyspace configuration: new UpdateKeyspaceConfig method and helper, UpdateKeyspaceConfigParams with pointer-valued maps and optional preconditions, a request-name constant, centralized PB conversion, request-aware retry behavior, and an integration test exercising set, precondition failure, and clear flows. ChangesKeyspace Config Update
Sequence DiagramsequenceDiagram
participant Caller
participant UpdateKeyspaceConfig
participant patchKeyspaceConfig
participant HTTPClient
Caller->>UpdateKeyspaceConfig: UpdateKeyspaceConfig(ctx, keyspaceName, params)
UpdateKeyspaceConfig->>patchKeyspaceConfig: delegate (ctx, keyspaceName, params)
patchKeyspaceConfig->>patchKeyspaceConfig: Marshal params to JSON
patchKeyspaceConfig->>HTTPClient: PATCH GetUpdateKeyspaceConfigURL(keyspaceName)
HTTPClient-->>patchKeyspaceConfig: response (updated keyspace meta) or error
patchKeyspaceConfig-->>UpdateKeyspaceConfig: return PB or error
UpdateKeyspaceConfig-->>Caller: return KeyspaceMeta or error
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)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10717 +/- ##
==========================================
+ Coverage 79.07% 79.09% +0.01%
==========================================
Files 536 536
Lines 73307 73690 +383
==========================================
+ Hits 57971 58287 +316
- Misses 11233 11285 +52
- Partials 4103 4118 +15
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/retest |
|
/retest |
|
|
||
| // UpdateKeyspaceConfig patches the keyspace config. | ||
| func (c *client) UpdateKeyspaceConfig(ctx context.Context, keyspaceName string, params *UpdateKeyspaceConfigParams) error { | ||
| return c.updateKeyspaceConfig(ctx, UpdateKeyspaceConfigName, keyspaceName, params) |
There was a problem hiding this comment.
Please consider the retry semantics for 409 Conflict here. A keyspace config precondition failure is returned by the server as 409, and this is a deterministic CAS failure, so it should not be retried against other PD members. The current HTTP client noNeedRetry only includes 400/403/404, so this PATCH will continue to be sent to subsequent endpoints after a 409. That can duplicate the same failed semantic request and logs. I suggest either disabling 409 retry for this API, or adding http.StatusConflict to noNeedRetry after confirming no other API relies on retrying 409.
There was a problem hiding this comment.
@lhy1024 Modifying noNeedRetry requires further discussion; it's better not to include it in this PR.
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
Signed-off-by: ystaticy <y_static_y@sina.com>
|
/retest-required |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bufferflies, lhy1024 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 |
[LGTM Timeline notifier]Timeline:
|
|
/retest |
|
/retest |
|
/test pull-unit-test-next-gen-3 |
|
/retest |
What problem does this PR solve?
Issue Number: Close #10718
What is changed and how does it work?
Check List
Tests
Code changes
Side effects
Related changes
pingcap/docs/pingcap/docs-cn:pingcap/tiup:Release note
Summary by CodeRabbit
New Features
Bug Fixes
Tests