storelimit: remove store limit ttlSecond param #10135
Conversation
Signed-off-by: okjiang <819421878@qq.com>
ti-chi-bot
bot
added
release-note-none
okJiang
changed the title
okJiang
changed the title
|
/retest |
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (35.71%) is below the target coverage (74.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #10135 +/- ##
==========================================
- Coverage 78.61% 78.58% -0.04%
==========================================
Files 520 520
Lines 69762 69650 -112
==========================================
- Hits 54842 54731 -111
- Misses 10968 10972 +4
+ Partials 3952 3947 -5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| AddPeer: sc.DefaultStoreLimit.GetDefaultStoreLimit(storelimit.AddPeer), | ||
| RemovePeer: sc.DefaultStoreLimit.GetDefaultStoreLimit(storelimit.RemovePeer), | ||
| } | ||
| v, ok1, err := o.getTTLFloat("default-add-peer") |
There was a problem hiding this comment.
How about rejecting this request if this parameter exists?
There was a problem hiding this comment.
I think we could return 400 and add a release note for it.
| return | ||
| } | ||
| var ttl int | ||
| if ttlSec := r.URL.Query().Get("ttlSecond"); ttlSec != "" { |
There was a problem hiding this comment.
Do you mean this parameter? @bufferflies @lhy1024
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughRemoved TTL-based store-limit functionality and related helpers/constants across scheduling, persistence, API, cluster, handler, and tests; store-limit operations now use direct in-memory defaults and non-TTL persistence. (50 words) Changes
Sequence Diagram(s)(omitted — changes are removals of TTL behavior and do not introduce a new multi-component sequential flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @pkg/mcs/scheduling/server/config/config.go:
- Around line 492-504: The API still accepts the deprecated ttlSecond parameter
even though PersistConfig.GetStoreLimit now always uses default limits; update
the API handler that processes store-limit/config requests to explicitly
validate the incoming payload and return HTTP 400 when ttlSecond is present,
remove the optional ttlSecond parameter from the client HTTP interface method
signature, and update any tests and call sites that pass ttlSecond to stop
sending it (and to assert the new default-limit behavior via
PersistConfig.GetStoreLimit and the store limit values it returns).
🧹 Nitpick comments (1)
server/config/persist_options.go (1)
474-486: Consider the side effect of state modification in a getter function.The implementation correctly shifts from TTL-based overrides to direct in-memory defaults. However, note that
GetStoreLimitnow has a side effect: it modifies the config by storing a new limit entry on first access to a store. While this is a one-time cost per store and ensures consistent defaults, the function name suggests it's read-only.This behavior might be intentional and matches the pattern of lazy initialization, but ensure this side effect is expected by callers, especially in concurrent scenarios where
SetScheduleConfiginvolves locking.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pkg/mcs/scheduling/server/config/config.gopkg/schedule/config/config.goserver/api/store.goserver/cluster/cluster.goserver/config/persist_options.goserver/handler.gotests/server/api/store_test.go
💤 Files with no reviewable changes (4)
- pkg/schedule/config/config.go
- server/handler.go
- server/cluster/cluster.go
- tests/server/api/store_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
server/config/persist_options.go (2)
pkg/schedule/config/config.go (3)
StoreLimitConfig(594-597)DefaultStoreLimit(98-98)StoreLimit(131-137)pkg/core/storelimit/limit.go (3)
AddPeer(26-26)RemovePeer(28-28)StoreLimit(51-65)
🔇 Additional comments (4)
server/config/persist_options.go (2)
489-502: LGTM!The function correctly delegates to
GetStoreLimitand extracts the appropriate rate based on the limit type. The removal of TTL-derived overrides is complete, and the logic now relies on direct in-memory configuration.
259-267: IsSupportedTTLConfig is correctly implemented.Verification confirms that
DefaultAddPeerandDefaultRemovePeerhave been cleanly removed from the supported TTL configs list and are not referenced anywhere in the codebase. The function logic is correct—it properly iterates through the remaining 12 supported TTL configuration keys and returns false when no match is found. No cleanup needed.pkg/mcs/scheduling/server/config/config.go (1)
507-520: LGTM! Simplified logic correctly delegates to GetStoreLimit.The function has been appropriately simplified to rely on
GetStoreLimit(storeID)without applying TTL-derived overrides. The switch statement correctly maps the store limit type to the corresponding field.server/api/store.go (1)
441-444: The removal of the TTL parameter fromSetAllStoresLimitandSetStoreLimitfunctions is complete and correct. The implementation at lines 441-444 is consistent with all other callers throughout the codebase, and no remaining references to TTL in the store limit context exist.All existing API clients using these functions continue to work without the TTL parameter, as demonstrated by test coverage across the codebase. The parameter order is correct for the Handler interface, and error handling is appropriate.
| func (o *PersistConfig) GetStoreLimit(storeID uint64) (returnSC sc.StoreLimitConfig) { | ||
| defer func() { | ||
| returnSC.RemovePeer = o.getTTLFloatOr(fmt.Sprintf("remove-peer-%v", storeID), returnSC.RemovePeer) | ||
| returnSC.AddPeer = o.getTTLFloatOr(fmt.Sprintf("add-peer-%v", storeID), returnSC.AddPeer) | ||
| }() | ||
| if limit, ok := o.GetScheduleConfig().StoreLimit[storeID]; ok { | ||
| return limit | ||
| } | ||
| cfg := o.GetScheduleConfig().Clone() | ||
| sc := sc.StoreLimitConfig{ | ||
| limitCfg := sc.StoreLimitConfig{ | ||
| AddPeer: sc.DefaultStoreLimit.GetDefaultStoreLimit(storelimit.AddPeer), | ||
| RemovePeer: sc.DefaultStoreLimit.GetDefaultStoreLimit(storelimit.RemovePeer), | ||
| } | ||
| v, ok1, err := o.getTTLFloat("default-add-peer") | ||
| if err != nil { | ||
| log.Warn("failed to parse default-add-peer from PersistOptions's ttl storage", zap.Error(err)) | ||
| } | ||
| canSetAddPeer := ok1 && err == nil | ||
| if canSetAddPeer { | ||
| returnSC.AddPeer = v | ||
| } | ||
|
|
||
| v, ok2, err := o.getTTLFloat("default-remove-peer") | ||
| if err != nil { | ||
| log.Warn("failed to parse default-remove-peer from PersistOptions's ttl storage", zap.Error(err)) | ||
| } | ||
| canSetRemovePeer := ok2 && err == nil | ||
| if canSetRemovePeer { | ||
| returnSC.RemovePeer = v | ||
| } | ||
|
|
||
| if canSetAddPeer || canSetRemovePeer { | ||
| return returnSC | ||
| } | ||
| cfg.StoreLimit[storeID] = sc | ||
| cfg.StoreLimit[storeID] = limitCfg | ||
| o.SetScheduleConfig(cfg) | ||
| return o.GetScheduleConfig().StoreLimit[storeID] | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the repository structure and find relevant files
git ls-files | head -20Repository: tikv/pd
Length of output: 609
🏁 Script executed:
#!/bin/bash
# Description: Verify claims about removed TTL-related functions and parameters
# Search for getTTLFloat usage (should find none if properly removed)
echo "=== Checking for getTTLFloat references ==="
rg -nP '\bgetTTLFloat\b' --type=go || echo "No getTTLFloat found (as expected)"
# Search for any remaining ttlSecond parameter usage
echo "=== Checking for ttlSecond parameter references ==="
rg -nP '\bttlSecond\b' --type=go || echo "No ttlSecond found (as expected)"
# Search for getTTLFloatOr (another removed function mentioned)
echo "=== Checking for getTTLFloatOr references ==="
rg -nP '\bgetTTLFloatOr\b' --type=go || echo "No getTTLFloatOr found"
# Look at the config file to understand the changes
echo "=== GetStoreLimit function context ==="
sed -n '492,504p' pkg/mcs/scheduling/server/config/config.goRepository: tikv/pd
Length of output: 1769
🏁 Script executed:
#!/bin/bash
# Search for store limit API endpoints/handlers
echo "=== Searching for SetStoreLimit API handlers ==="
rg -nP 'SetStoreLimit|storeLimit.*(?:handler|Handler|request|Request)' --type=go -A 5 | head -50
# Search for any API endpoint files in the scheduling package
echo "=== Finding API-related files in scheduling package ==="
fd -e go 'api|handler|request' pkg/mcs/scheduling/ | head -20Repository: tikv/pd
Length of output: 4488
🏁 Script executed:
#!/bin/bash
# Check the full GetStoreLimit and GetStoreLimitByType functions
echo "=== GetStoreLimit and GetStoreLimitByType functions ==="
sed -n '492,520p' pkg/mcs/scheduling/server/config/config.go
# Look for DefaultStoreLimit to verify it's properly configured
echo "=== DefaultStoreLimit references ==="
rg -nP 'DefaultStoreLimit' --type=go -B 2 -A 2 | head -40Repository: tikv/pd
Length of output: 3921
🏁 Script executed:
#!/bin/bash
# Search for ttlSecond in all files (not just Go)
echo "=== Broader search for ttlSecond ==="
rg -n 'ttlSecond' -i | head -30 || echo "No ttlSecond found"
# Look for past review comments or discussion about ttlSecond parameter
echo "=== Search for API validation patterns ==="
rg -nP 'StoreLimit.*ttl|ttl.*StoreLimit' --type=go -C 3 || echo "No mixed TTL/StoreLimit patterns found"Repository: tikv/pd
Length of output: 2949
LGTM! Default store limits are properly applied.
The refactoring correctly simplifies the logic by removing TTL-driven overrides and using default store limits directly. The use of Clone() ensures thread-safety when modifying the config.
Note: The API layer still accepts the ttlSecond parameter (see server/api/config.go:144), with a deprecation comment indicating it's only for BR and lightning. If the intention is to fully remove support for this parameter, the API layer should be updated to explicitly reject requests containing ttlSecond with a 400 error, and the client API (client/http/interface.go:475) should remove the optional ttlSecond parameter. Additionally, update related tests (e.g., tests/server/config/config_test.go:514 and tests/integrations/client/client_test.go:934) to not include the deprecated parameter.
Verify that all callers expecting TTL-based behavior have been updated to use the new default store limit approach.
🤖 Prompt for AI Agents
In @pkg/mcs/scheduling/server/config/config.go around lines 492 - 504, The API
still accepts the deprecated ttlSecond parameter even though
PersistConfig.GetStoreLimit now always uses default limits; update the API
handler that processes store-limit/config requests to explicitly validate the
incoming payload and return HTTP 400 when ttlSecond is present, remove the
optional ttlSecond parameter from the client HTTP interface method signature,
and update any tests and call sites that pass ttlSecond to stop sending it (and
to assert the new default-limit behavior via PersistConfig.GetStoreLimit and the
store limit values it returns).
[LGTM Timeline notifier]Timeline:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @server/api/store.go:
- Around line 322-324: The handler checks for the deprecated parameter ttlSecond
and writes a 400 via h.rd.JSON(w, http.StatusBadRequest, "ttlSecond is
deprecated") but does not stop execution; add an immediate return after that
response to prevent further processing of the request (i.e., after the ttlSec
check and h.rd.JSON call, return from the current handler function so no further
code runs).
- Around line 418-420: The handler checks the deprecated query param ttlSecond
and writes a 400 response but doesn't stop execution; update the ttlSecond
handling in the same function to return immediately after calling h.rd.JSON(w,
http.StatusBadRequest, "ttlSecond is deprecated") so the rest of the handler
does not run (mirror the early-return behavior used in SetStoreLimit).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/api/store.go
🧰 Additional context used
🧬 Code graph analysis (1)
server/api/store.go (2)
pkg/core/store_option.go (1)
SetStoreLimit(298-304)server/handler.go (1)
Handler(73-79)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: chunks
- GitHub Check: tso-function-test
- GitHub Check: chunks (8, TSO Integration Test)
- GitHub Check: chunks (3, Unit Test(3))
- GitHub Check: chunks (2, Unit Test(2))
- GitHub Check: chunks (1, Unit Test(1))
- GitHub Check: chunks (4, Tests(1))
- GitHub Check: chunks (6, Tools Test)
- GitHub Check: chunks (5, Tests(2))
- GitHub Check: statics
- GitHub Check: chunks (9, Microservice Integration(!TSO))
- GitHub Check: chunks (10, Microservice Integration(TSO))
- GitHub Check: chunks (7, Client Integration Test)
🔇 Additional comments (1)
server/api/store.go (1)
451-454: LGTM - Simplified flow correctly removes TTL logic.The removal of TTL-based branching and the direct call to
SetAllStoresLimitwithout TTL parameters correctly implements the deprecation. Error handling is appropriate.
Signed-off-by: okjiang <819421878@qq.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/api/store.go (1)
322-325: Consider enhancing the deprecation error message.The implementation correctly rejects the deprecated parameter, but the error message could be more helpful by providing migration guidance.
💬 Suggested improvement for error message
if ttlSec := r.URL.Query().Get("ttlSecond"); ttlSec != "" { - h.rd.JSON(w, http.StatusBadRequest, "ttlSecond is deprecated") + h.rd.JSON(w, http.StatusBadRequest, "ttlSecond is deprecated and no longer supported; please remove this parameter from your request") return }Verify: Is immediate rejection (vs. gradual deprecation) intended?
This implementation returns an error immediately when
ttlSecondis present, which is a breaking change rather than a gradual deprecation. According to the PR description, BR/Lightning don't use ttlSecond in SetStoreLimit, so the impact may be acceptable. However, please confirm this aligns with your API versioning and backward compatibility policy.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/api/store.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: chunks (9, Microservice Integration(!TSO))
- GitHub Check: chunks (1, Unit Test(1))
- GitHub Check: chunks (7, Client Integration Test)
- GitHub Check: chunks (10, Microservice Integration(TSO))
- GitHub Check: chunks (8, TSO Integration Test)
- GitHub Check: chunks (3, Unit Test(3))
- GitHub Check: chunks (4, Tests(1))
- GitHub Check: chunks (5, Tests(2))
- GitHub Check: chunks (2, Unit Test(2))
- GitHub Check: chunks (6, Tools Test)
- GitHub Check: statics
- GitHub Check: tso-function-test
- GitHub Check: chunks
🔇 Additional comments (3)
server/api/store.go (3)
314-314: LGTM: Documentation correctly marks ttlSecond as deprecated.The OpenAPI documentation updates appropriately indicate that the ttlSecond parameter is deprecated in both endpoints.
Also applies to: 411-411
419-422: Same deprecation pattern as SetStoreLimit.The deprecation check here follows the same pattern as in SetStoreLimit (lines 322-325). The same suggestions regarding the error message and verification of the breaking change approach apply here as well.
453-456: LGTM: Simplified to non-TTL API call.The call to
SetAllStoresLimithas been correctly simplified to remove TTL parameters, consistent with the deprecation of ttlSecond functionality.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bufferflies, 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 |
close tikv#4399 Signed-off-by: okjiang <819421878@qq.com>
close tikv#4399 Signed-off-by: okjiang <819421878@qq.com>
5 participants
What problem does this PR solve?
Issue Number: Close #4399
What is changed and how does it work?
remove useless code, BR/Lightning only use ttlSecond in
SetConfig, not useSetStoreLimitCheck List
Tests
Code changes
Side effects
Related changes
pingcap/docs/pingcap/docs-cn:pingcap/tiup:Release note
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.