[release-8.5-keyspace] add option to skip rawkv region bound#10535
[release-8.5-keyspace] add option to skip rawkv region bound#10535bufferflies wants to merge 7 commits intotikv:masterfrom
Conversation
… (tikv#358) * [release 8.1] cp disable-raw-kv-region-split (tikv#292) * Add option to skip split rawkv regions (tikv#217) * add option to skip rawkv region bound Signed-off-by: AmoebaProtozoa <8039876+AmoebaProtozoa@users.noreply.github.com> --------- Signed-off-by: AmoebaProtozoa <8039876+AmoebaProtozoa@users.noreply.github.com> * make check Signed-off-by: zeminzhou <zhouzemin@pingcap.com> --------- Signed-off-by: AmoebaProtozoa <8039876+AmoebaProtozoa@users.noreply.github.com> Signed-off-by: zeminzhou <zhouzemin@pingcap.com> Co-authored-by: David <8039876+AmoebaProtozoa@users.noreply.github.com> (cherry picked from commit 0c88ce8)
ti-chi-bot
bot
added
dco-signoff: yes
|
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:
📝 WalkthroughWalkthroughThis PR refactors keyspace region bound handling by introducing a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 |
ti-chi-bot
bot
added
the
size/L
|
Review note: The new Please change the tag to |
|
Re-review update: I rechecked the latest delta and the previous finding is resolved.
I do not have additional findings on this update. The remaining merge gate is the required |
![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
🧹 Nitpick comments (1)
pkg/keyspace/keyspace_test.go (1)
66-92: Add one manager-level test that actually setsDisableRawKVRegionSplit=true.This mock wiring keeps the suite compiling, but nothing in this file exercises the new
waitKeyspaceRegionSplit/CheckKeyspaceRegionBoundbranch with raw bounds absent. A txn-only-ready case here would catch the behavior this PR is changing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/keyspace/keyspace_test.go` around lines 66 - 92, The test suite never sets DisableRawKVRegionSplit on the mockConfig so the new branches in waitKeyspaceRegionSplit and CheckKeyspaceRegionBound are not exercised; add a manager-level test that creates/uses mockConfig with DisableRawKVRegionSplit=true (via GetDisableRawKVRegionSplit) and drives the manager code paths that call waitKeyspaceRegionSplit and CheckKeyspaceRegionBound (simulate/prepare a txn-only-ready keyspace with raw bounds missing) so the branch that handles raw-bounds-absent behavior executes and asserts expected outcome.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/keyspace/util.go`:
- Around line 129-130: The rename of makeKeyRanges to an unexported identifier
breaks external callers; either restore an exported wrapper MakeKeyRanges(id
uint32, skipRaw bool) that calls the new makeKeyRanges, or update all external
call sites to use the unexported symbol (preferred if migrating within the same
package). Add an exported function named MakeKeyRanges that forwards its
arguments to makeKeyRanges (preserving signature and behavior) so tools/pd-ctl
callers (e.g., keyspace.MakeKeyRanges) keep building, or alternatively change
those callers to call the new unexported implementation and remove the wrapper.
---
Nitpick comments:
In `@pkg/keyspace/keyspace_test.go`:
- Around line 66-92: The test suite never sets DisableRawKVRegionSplit on the
mockConfig so the new branches in waitKeyspaceRegionSplit and
CheckKeyspaceRegionBound are not exercised; add a manager-level test that
creates/uses mockConfig with DisableRawKVRegionSplit=true (via
GetDisableRawKVRegionSplit) and drives the manager code paths that call
waitKeyspaceRegionSplit and CheckKeyspaceRegionBound (simulate/prepare a
txn-only-ready keyspace with raw bounds missing) so the branch that handles
raw-bounds-absent behavior executes and asserts expected outcome.
🪄 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: db1a2dcf-698f-41aa-b67d-e062dea57b9f
📒 Files selected for processing (6)
pkg/keyspace/keyspace.gopkg/keyspace/keyspace_test.gopkg/keyspace/util.gopkg/keyspace/util_test.gopkg/mcs/scheduling/server/rule/watcher_test.goserver/config/config.go
Signed-off-by: bufferflies <1045931706@qq.com>
bufferflies
force-pushed
the
pr-merge/0c88ce89-skip-rawkv-region-bound
branch
from
cea718f to
4473520
Compare
|
Review note:
Even if current call sites are internal, this helper was previously exported and had a concrete return type. Please keep the exported helper and preserve the |
Signed-off-by: bufferflies <1045931706@qq.com>
|
Review note: This is only a partial compatibility fix. That is still a breaking API change for any existing caller that uses the old one-argument form. If compatibility is the goal, the old exported signature should remain available. A safer shape would be to keep |
Signed-off-by: bufferflies <1045931706@qq.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/pd-ctl/pdctl/command/keyspace_command_test.go (1)
41-74: Consider adding test coverage forskipRaw=true.The test correctly validates the
skipRaw=falsebehavior. However, the newskipRaw=truepath is not covered in this test file. Adding a test case that verifiesMakeKeyRanges(keyspaceID, true)returns only 1 range (txn-only) would improve coverage of the new functionality.💡 Example test case for skipRaw=true
func TestMakeKeyRangesSkipRaw(t *testing.T) { re := require.New(t) keyspaceID := uint32(1) ranges := keyspace.MakeKeyRanges(keyspaceID, true) re.Len(ranges, 1, "should have 1 range (txn only) when skipRaw=true") // Verify txn key range txnRange := ranges[0].(map[string]any) txnStart := txnRange["start_key"].(string) txnEnd := txnRange["end_key"].(string) re.NotEmpty(txnStart) re.NotEmpty(txnEnd) // Verify the encoding matches the expected txn format keyspaceIDBytes := make([]byte, 4) nextKeyspaceIDBytes := make([]byte, 4) binary.BigEndian.PutUint32(keyspaceIDBytes, keyspaceID) binary.BigEndian.PutUint32(nextKeyspaceIDBytes, keyspaceID+1) expectedTxnLeft := codec.EncodeBytes(append([]byte{'x'}, keyspaceIDBytes[1:]...)) expectedTxnRight := codec.EncodeBytes(append([]byte{'x'}, nextKeyspaceIDBytes[1:]...)) re.Equal(hex.EncodeToString(expectedTxnLeft), txnStart) re.Equal(hex.EncodeToString(expectedTxnRight), txnEnd) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/pd-ctl/pdctl/command/keyspace_command_test.go` around lines 41 - 74, Add a unit test covering the new skipRaw=true path: call keyspace.MakeKeyRanges(keyspaceID, true) in keyspace_command_test.go (similar structure to existing loop test) and assert it returns exactly 1 range (txn-only); verify the returned map uses keys "start_key"/"end_key" and that encoded txn boundaries equal codec.EncodeBytes(append([]byte{'x'}, ...)) for keyspaceID and keyspaceID+1 as done in the existing assertions for the txn range.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tools/pd-ctl/pdctl/command/keyspace_command_test.go`:
- Around line 41-74: Add a unit test covering the new skipRaw=true path: call
keyspace.MakeKeyRanges(keyspaceID, true) in keyspace_command_test.go (similar
structure to existing loop test) and assert it returns exactly 1 range
(txn-only); verify the returned map uses keys "start_key"/"end_key" and that
encoded txn boundaries equal codec.EncodeBytes(append([]byte{'x'}, ...)) for
keyspaceID and keyspaceID+1 as done in the existing assertions for the txn
range.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c0f0db76-96c4-42eb-b36e-91f8e6678652
📒 Files selected for processing (4)
pkg/keyspace/keyspace.gopkg/keyspace/util.gotools/pd-ctl/pdctl/command/keyspace_command.gotools/pd-ctl/pdctl/command/keyspace_command_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/keyspace/keyspace.go
|
Review note: There is still another compatibility regression in the same area: Even though the current discussion focused on |
Signed-off-by: bufferflies <1045931706@qq.com>
|
/check-issue-triage-complete |
|
Re-review update: I rechecked the latest delta and the
I do not have an additional compatibility finding on this specific follow-up change. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10535 +/- ##
==========================================
+ Coverage 78.88% 78.96% +0.08%
==========================================
Files 530 531 +1
Lines 71548 71655 +107
==========================================
+ Hits 56439 56584 +145
+ Misses 11092 11057 -35
+ Partials 4017 4014 -3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/ping @AmoebaProtozoa @rleungx |
| @@ -576,7 +577,15 @@ func (manager *Manager) waitKeyspaceRegionSplit(id uint32) error { | |||
|
|
|||
| // CheckKeyspaceRegionBound checks whether the keyspace region has been split. | |||
| func (manager *Manager) CheckKeyspaceRegionBound(id uint32) bool { | |||
There was a problem hiding this comment.
If a keyspace created with disable-raw-kv-region-split=true only has txn bounds. If the config is later changed to false, it may never pass CheckKeyspaceRegionBound again and can become effectively invisible to LoadKeyspace.
There was a problem hiding this comment.
Yes, the existing keyspace isn't affected by the configuration by design, and later add notes on the user docs.
There was a problem hiding this comment.
If this is intended to be immutable after keyspace creation, can we enforce that in code (for example, reject dynamic updates for this field)?
There was a problem hiding this comment.
good catch, I will create new issue to follow it #10572
There was a problem hiding this comment.
Do we need to add TODO here?
|
@AmoebaProtozoa: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn 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. |
ti-chi-bot
bot
added
the
release-note
ti-chi-bot
bot
removed
the
release-note-none
[LGTM Timeline notifier]Timeline:
|
server/config/config.go
Outdated
| defaultGCTunerThreshold = 0.6 | ||
| minGCTunerThreshold = 0 | ||
| maxGCTunerThreshold = 0.9 | ||
| defaultDisableRawKVRegionSplit = false |
There was a problem hiding this comment.
It may introduce configuration compatibility issues.
There was a problem hiding this comment.
But usually we use enablexxx for a switch.
Signed-off-by: bufferflies <1045931706@qq.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AmoebaProtozoa, 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 |
| } | ||
|
|
||
| func (manager *Manager) getRegionBoundType() regionBoundType { | ||
| if manager.cluster == nil || manager.cluster.GetSharedConfig() == nil { |
There was a problem hiding this comment.
This fallback looks risky. If cluster exists but GetSharedConfig() is temporarily nil / unavailable, we now silently fall back to table -> txn bound, which changes the old behavior from "check all bounds" to txn-only. That can misclassify non-table deployments and generate the wrong label rule / split check before shared config is ready. Please either keep an explicit "unknown -> allRegionBound" fallback here, or make the caller fail closed until the key type is available instead of defaulting to Table.
Signed-off-by: bufferflies <1045931706@qq.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/keyspace/keyspace.go (1)
579-604:⚠️ Potential issue | 🟠 MajorDon’t re-derive region bounds from the live shared config.
CheckKeyspaceRegionBoundnow uses the currentGetSharedConfig().GetKeyType()every time. That means a keyspace created with txn-only bounds can later failLoadKeyspaceif the shared config changes, and thenilfallback here silently pickstxnRegionBoundas well. This contradicts the “only new keyspaces are affected” behavior described for the feature. The bound type needs to come from the keyspace’s creation-time choice (or other persisted metadata), not from the current cluster config.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/keyspace/keyspace.go` around lines 579 - 604, CheckKeyspaceRegionBound currently derives the region bound type from live config via getRegionBoundType(), which can change over time; instead use the keyspace's persisted creation-time bound metadata (e.g., a field or method on the keyspace record) when deciding bounds. Update CheckKeyspaceRegionBound/hasKeyspaceRegionBound to fetch the stored bound type from the keyspace object (for example use keyspace.RegionBoundType or keyspace.GetRegionBoundType()) rather than calling manager.getRegionBoundType(), and remove/adjust the nil/shared-config fallback so it never re-derives bounds from the cluster config at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/keyspace/keyspace.go`:
- Around line 579-604: CheckKeyspaceRegionBound currently derives the region
bound type from live config via getRegionBoundType(), which can change over
time; instead use the keyspace's persisted creation-time bound metadata (e.g., a
field or method on the keyspace record) when deciding bounds. Update
CheckKeyspaceRegionBound/hasKeyspaceRegionBound to fetch the stored bound type
from the keyspace object (for example use keyspace.RegionBoundType or
keyspace.GetRegionBoundType()) rather than calling manager.getRegionBoundType(),
and remove/adjust the nil/shared-config fallback so it never re-derives bounds
from the cluster config at runtime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63def982-4199-4bbe-a19e-ed88981d1090
📒 Files selected for processing (3)
pkg/keyspace/keyspace.gopkg/keyspace/util.gopkg/keyspace/util_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/keyspace/util.go
|
@bufferflies: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
5 participants
What problem does this PR solve?
Issue Number: ref #10516,close #10534
author: @zeminzhou
cp
0c88ce89What is changed and how does it work?
Cherry-pick
0c88ce8921caabfa532e4a2a259bf39f7a49bdd4ontomasterand adapt the keyspace region-bound path to the current code structure.This follow-up removes the temporary
disable-raw-kv-region-splitswitch and derives the bound type directly fromkey-type:key-type=table-> use txn keyspace boundskey-type=raw-> use raw keyspace boundskey-type=txn-> use raw keyspace boundsThe exported helper surface stays compatible, while the keyspace manager now routes label-rule generation and bound checks through the internal key-type mapping.
Check List
Tests
Release note
Summary by CodeRabbit