keyspace, schedule: make keyspace creation atomic #10492
Conversation
Signed-off-by: tongjian <1045931706@qq.com>
Signed-off-by: tongjian <1045931706@qq.com>
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
the
size/XXL
|
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 creation and keyspace-group membership around transactional operations and post-commit cache updates, changes meta-service-group counting APIs, dynamically resolves integration-test keyspace IDs, and adds RegionLabeler storage helpers. ChangesKeyspace transaction workflows
Labeler validation and storage helpers
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant KeyspaceManager
participant Store
participant RegionLabeler
Client->>KeyspaceManager: CreateKeyspace(name, config)
KeyspaceManager->>Store: Commit metadata, group assignment, and region-label rules
Store-->>KeyspaceManager: Transaction committed
KeyspaceManager->>RegionLabeler: SaveRuleWithoutTxn
KeyspaceManager-->>Client: Return keyspace metadata
sequenceDiagram
participant Caller
participant GroupManager
participant Store
Caller->>GroupManager: updateKeyspaceForGroupTxnOp
GroupManager->>Store: Commit group membership transaction
Store-->>GroupManager: Transaction result
GroupManager->>Store: Reload committed group
Store-->>GroupManager: Updated membership
GroupManager->>GroupManager: Refresh cache
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
/run-check-issue-triage-complete |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pkg/keyspace/metrics.go (1)
17-25: Import order does not follow gci/goimports convention.The imports should follow: stdlib | third-party | pingcap | tikv/pd. Currently
github.com/tikv/pd/pkg/storage/endpoint(line 21) is placed between third-party imports (prometheusandzap).As per coding guidelines: "Use gci/goimports ordering: stdlib | third-party | pingcap | tikv/pd".
♻️ Suggested import reordering
import ( "time" "github.com/prometheus/client_golang/prometheus" - "github.com/tikv/pd/pkg/storage/endpoint" "go.uber.org/zap" "github.com/pingcap/log" + + "github.com/tikv/pd/pkg/storage/endpoint" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/keyspace/metrics.go` around lines 17 - 25, Reorder the imports in pkg/keyspace/metrics.go to follow the gci/goimports convention (stdlib | third-party | pingcap | tikv/pd): keep "time" first, group third-party imports ("github.com/prometheus/client_golang/prometheus", "go.uber.org/zap") next, then pingcap ("github.com/pingcap/log"), and finally tikv/pd ("github.com/tikv/pd/pkg/storage/endpoint"); adjust the import block ordering accordingly and run goimports/gci to verify formatting.server/apiv2/handlers/keyspace.go (1)
174-179: Consider extracting duplicated state adjustment logic.The same logic for adjusting keyspace state appears in both
LoadKeyspace(lines 174-179) andLoadKeyspaceByID(lines 209-213). Consider extracting to a helper function to reduce duplication.Also note: mutating
meta.Statedirectly modifies the proto object. Verify this doesn't affect any in-memory caches that might hold a reference to the same object returned byLoadKeyspace.♻️ Suggested helper extraction
// adjustKeyspaceStateForResponse adjusts the keyspace state for API response // if region split is pending. func adjustKeyspaceStateForResponse(meta *keyspacepb.KeyspaceMeta, svr *server.Server, manager *keyspace.Manager) { if meta != nil && meta.State == keyspacepb.KeyspaceState_ENABLED { if svr.GetKeyspaceConfig().ToWaitRegionSplit() && !manager.CheckKeyspaceRegionBound(meta.Id) { meta.State = keyspacepb.KeyspaceState_DISABLED } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/apiv2/handlers/keyspace.go` around lines 174 - 179, The duplicated logic that mutates meta.State in LoadKeyspace and LoadKeyspaceByID should be extracted into a helper (e.g., adjustKeyspaceStateForResponse) that takes the *keyspacepb.KeyspaceMeta, *server.Server and *keyspace.Manager and encapsulates the ToWaitRegionSplit()/CheckKeyspaceRegionBound(meta.Id) check and sets State to DISABLED for responses; to avoid mutating shared in-memory proto caches, ensure you operate on a defensive copy of meta (clone the proto or create a shallow copy) before calling the helper so the in-memory cached meta is not modified.
🤖 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/keyspace.go`:
- Around line 185-197: initReserveKeyspace currently returns any error from
createKeyspaceWithoutCheck, which now returns ErrKeyspaceExists and causes
bootstrap to fail on restart; update initReserveKeyspace to treat
ErrKeyspaceExists as benign by checking the returned error from
createKeyspaceWithoutCheck and returning nil when err == ErrKeyspaceExists (but
return other errors normally), keeping the rest of the tracing and logic
unchanged; reference createKeyspaceWithoutCheck, ErrKeyspaceExists and
initReserveKeyspace when making the change.
- Around line 232-248: The CreateKeyspaceByID path currently skips validateID()
when caller-supplied ids are used; update the branch that sets newID = ids[0] to
call validateID(newID) and return an error if validation fails (in the same
manner as the allocator path), before calling tracer.SetKeyspace and
isProtectedKeyspaceID; apply the same change to the other similar branch around
the CreateKeyspaceByID/ids handling (the one at the 363–367 site). Use the
existing validateID, isProtectedKeyspaceID, and manager.allocID symbols to
locate and mirror the allocator-path checks so out-of-range or otherwise invalid
supplied IDs are rejected consistently.
In `@pkg/keyspace/tso_keyspace_group.go`:
- Around line 464-512: updateKeyspaceForGroupTxnOp currently mutates the live
cache entry kg before the txn runs, leaking uncommitted changes; instead, clone
the group (e.g., make a deep copy of kg), apply the opAdd/opDelete mutation to
the clone and pass that clone into saveKeyspaceGroupsTxnOp, and do not touch
m.groups while unlocked; in the returned callback (cb) only on success reacquire
m.Lock() and call m.groups[userKind].Put(clonedKg) to publish the change, and on
error do nothing (or explicitly discard the clone) so the live kg is never
mutated outside the committed-path. Ensure kg (the original from m.groups) is
never modified in updateKeyspaceForGroupTxnOp before the transaction runs.
---
Nitpick comments:
In `@pkg/keyspace/metrics.go`:
- Around line 17-25: Reorder the imports in pkg/keyspace/metrics.go to follow
the gci/goimports convention (stdlib | third-party | pingcap | tikv/pd): keep
"time" first, group third-party imports
("github.com/prometheus/client_golang/prometheus", "go.uber.org/zap") next, then
pingcap ("github.com/pingcap/log"), and finally tikv/pd
("github.com/tikv/pd/pkg/storage/endpoint"); adjust the import block ordering
accordingly and run goimports/gci to verify formatting.
In `@server/apiv2/handlers/keyspace.go`:
- Around line 174-179: The duplicated logic that mutates meta.State in
LoadKeyspace and LoadKeyspaceByID should be extracted into a helper (e.g.,
adjustKeyspaceStateForResponse) that takes the *keyspacepb.KeyspaceMeta,
*server.Server and *keyspace.Manager and encapsulates the
ToWaitRegionSplit()/CheckKeyspaceRegionBound(meta.Id) check and sets State to
DISABLED for responses; to avoid mutating shared in-memory proto caches, ensure
you operate on a defensive copy of meta (clone the proto or create a shallow
copy) before calling the helper so the in-memory cached meta is not modified.
🪄 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: 6b76ad37-a633-4470-96f6-ef77ae5c598b
📥 Commits
Reviewing files that changed from the base of the PR and between 3eb99ae and 5ef55c918795292e83aea368b1c0febdc7d2d965.
📒 Files selected for processing (10)
pkg/keyspace/keyspace.gopkg/keyspace/keyspace_test.gopkg/keyspace/metrics.gopkg/keyspace/tso_keyspace_group.gopkg/keyspace/tso_keyspace_group_test.gopkg/schedule/labeler/labeler.gopkg/schedule/labeler/labeler_test.gopkg/schedule/labeler/plan.gopkg/schedule/labeler/rules.goserver/apiv2/handlers/keyspace.go
5ef55c9 to
9c634d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
pkg/keyspace/keyspace.go (2)
233-248:⚠️ Potential issue | 🟠 MajorValidate caller-supplied IDs before creating the keyspace.
The
idsbranch at Lines 233-235 bypassesvalidateID(), soCreateKeyspaceByID()can persist IDs thatallocID()would reject. Reuse the same validation beforeSetKeyspace()and the protected-ID check.💡 Minimal fix
if len(ids) > 0 { newID = ids[0] + if err := validateID(newID); err != nil { + return nil, err + } } else { // Allocate new keyspaceID. newID, err = manager.allocID()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/keyspace/keyspace.go` around lines 233 - 248, The caller-supplied branch bypasses validation: after assigning newID (whether from ids[0] or manager.allocID()), call the existing validateID(newID) helper and return its error if it fails before invoking tracer.SetKeyspace(newID, name) and before the isProtectedKeyspaceID(newID) check; in short, centralize validation on newID (used by CreateKeyspaceByID path) immediately after newID is chosen so both supplied IDs and allocated IDs go through validateID() prior to SetKeyspace() and the protected-ID error logic.
185-197:⚠️ Potential issue | 🔴 CriticalKeep reserved-keyspace bootstrap idempotent.
At Line 196,
initReserveKeyspace()now propagateserrs.ErrKeyspaceExistsfromcreateKeyspaceWithoutCheck(). On restart/recovery the reserved keyspace is already persisted, so this turns a normal bootstrap into a startup failure. Please treatErrKeyspaceExistsas benign here, the same way the pre-alloc loop already does.💡 Minimal fix
_, err = manager.createKeyspaceWithoutCheck(tracer, config, now) - return err + if err != nil && err != errs.ErrKeyspaceExists { + return err + } + return nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/keyspace/keyspace.go` around lines 185 - 197, initReserveKeyspace currently returns errs.ErrKeyspaceExists from createKeyspaceWithoutCheck which makes normal restarts fail; modify Manager.initReserveKeyspace to treat errs.ErrKeyspaceExists as benign by checking the returned error from manager.createKeyspaceWithoutCheck(tracer, config, now) and returning nil when the error equals errs.ErrKeyspaceExists, otherwise returning the original error; reference the functions Manager.initReserveKeyspace and createKeyspaceWithoutCheck and the sentinel error errs.ErrKeyspaceExists in your change.pkg/keyspace/tso_keyspace_group.go (1)
488-516:⚠️ Potential issue | 🔴 CriticalKeep
updateKeyspaceForGroupTxnOpside-effect free until commit.At Line 490,
kgis the live cache entry fromm.groups, but it is mutated before the returned txn runs. If a later setup step aborts beforeRunInTxn, or another request interleaves on the same group, uncommitted membership leaks into memory; then Lines 503-513 mutate the same shared state again without holdingm.Lock(). Clone the group, persist the clone, and only publish it from a success callback that reacquires the lock.As per coding guidelines, "Guard shared state with mutex/RWMutex; keep lock ordering consistent."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/keyspace/tso_keyspace_group.go` around lines 488 - 516, The live group entry kg is being mutated before the transaction runs; instead, make updateKeyspaceForGroupTxnOp operate on a deep copy (e.g., clone := kg.Clone() or manual copy) and apply the add/delete changes to the clone only, build the txn op from the clone, and in the success callback reacquire m.Lock()/m.Unlock() and call m.groups[userKind].Put(clone) to publish the committed change; ensure the rollback branch does not mutate the original shared kg and that any temporary mutation touches only the cloned object.
🤖 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/keyspace_test.go`:
- Around line 143-150: The failpoint injection for "waitSplitKeyspaceFailed" is
behind a dead branch because manager is constructed with &mockConfig{} in
SetupTest which makes ToWaitRegionSplit() return false; update the test so the
manager runs the wait-split path (either by configuring a non-zero wait-split
setting on the mockConfig used in SetupTest or constructing manager with a
config where ToWaitRegionSplit() returns true) before enabling the failpoint, or
move the failpoint enable to a code path that always executes (e.g., immediately
before calling CreateKeyspace or inside the commit-handling path); ensure the
test calls CreateKeyspace and exercises the post-commit split path so the
failpoint actually triggers.
In `@pkg/keyspace/keyspace.go`:
- Around line 313-349: The code must not return an error after
manager.store.RunInTxn (the transaction that persisted keyspace metadata) has
committed; modify the post-commit split logic in the function that calls
manager.store.RunInTxn / tracer.OnSaveKeyspaceMetaFinished so that failures from
waitKeyspaceRegionSplit(keyspaceID) are treated as best-effort: do not return
nil,err after the txn has succeeded. Instead either (A) swallow the wait error
(log.Warn with details and set a "split pending" condition on the returned
keyspace result or an explicit status field) and return the created keyspace and
nil error, or (B) change the function's result type to include a status enum
(e.g., Created|CreatedSplitPending) and return that along with the keyspace when
waitKeyspaceRegionSplit fails. Ensure callers are updated to handle the new
status or the guaranteed non-error return when the keyspace is durable.
---
Duplicate comments:
In `@pkg/keyspace/keyspace.go`:
- Around line 233-248: The caller-supplied branch bypasses validation: after
assigning newID (whether from ids[0] or manager.allocID()), call the existing
validateID(newID) helper and return its error if it fails before invoking
tracer.SetKeyspace(newID, name) and before the isProtectedKeyspaceID(newID)
check; in short, centralize validation on newID (used by CreateKeyspaceByID
path) immediately after newID is chosen so both supplied IDs and allocated IDs
go through validateID() prior to SetKeyspace() and the protected-ID error logic.
- Around line 185-197: initReserveKeyspace currently returns
errs.ErrKeyspaceExists from createKeyspaceWithoutCheck which makes normal
restarts fail; modify Manager.initReserveKeyspace to treat
errs.ErrKeyspaceExists as benign by checking the returned error from
manager.createKeyspaceWithoutCheck(tracer, config, now) and returning nil when
the error equals errs.ErrKeyspaceExists, otherwise returning the original error;
reference the functions Manager.initReserveKeyspace and
createKeyspaceWithoutCheck and the sentinel error errs.ErrKeyspaceExists in your
change.
In `@pkg/keyspace/tso_keyspace_group.go`:
- Around line 488-516: The live group entry kg is being mutated before the
transaction runs; instead, make updateKeyspaceForGroupTxnOp operate on a deep
copy (e.g., clone := kg.Clone() or manual copy) and apply the add/delete changes
to the clone only, build the txn op from the clone, and in the success callback
reacquire m.Lock()/m.Unlock() and call m.groups[userKind].Put(clone) to publish
the committed change; ensure the rollback branch does not mutate the original
shared kg and that any temporary mutation touches only the cloned object.
🪄 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: 7e277696-956c-4189-b34f-4704216ffc40
📥 Commits
Reviewing files that changed from the base of the PR and between 5ef55c918795292e83aea368b1c0febdc7d2d965 and 9c634d1.
📒 Files selected for processing (6)
pkg/keyspace/keyspace.gopkg/keyspace/keyspace_test.gopkg/keyspace/metrics.gopkg/keyspace/tso_keyspace_group.gopkg/keyspace/tso_keyspace_group_test.goserver/apiv2/handlers/keyspace.go
✅ Files skipped from review due to trivial changes (1)
- server/apiv2/handlers/keyspace.go
e9b0b94 to
693a40b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/keyspace/keyspace.go (1)
345-357:⚠️ Potential issue | 🟠 MajorDon't report
CreateKeyspacefailure after the txn has already committed.Once the transaction at line 321 succeeds, the keyspace meta, keyspace-group membership, and label rule are already durable. If
waitKeyspaceRegionSplit()then fails at line 346, the caller getsnil, erreven though the keyspace exists. A retry will hitErrKeyspaceExists.Either make the wait best-effort after commit (log and continue), or return a result that lets callers distinguish "created, split pending" from a real rollback.
💡 Suggested approach: treat split-wait as best-effort
// Split keyspace region. if tracer.waitSplit { err = manager.waitKeyspaceRegionSplit(tracer.keyspaceID) failpoint.Inject("waitSplitKeyspaceFailed", func() { err = errors.New("failpoint triggered: waitSplitKeyspaceFailed") }) if err != nil { log.Warn("[create-keyspace] failed to wait keyspace region split", zap.Uint32("keyspace-id", keyspace.GetId()), zap.String("keyspace-name", keyspace.GetName()), zap.Error(err), ) - return nil, err + // Keyspace is already persisted; treat split-wait failure as best-effort. + // The keyspace is usable; splits will eventually occur. } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/keyspace/keyspace.go` around lines 345 - 357, The CreateKeyspace path currently returns an error if manager.waitKeyspaceRegionSplit(tracer.keyspaceID) fails after the transaction has committed; change this to treat the split-wait as best-effort: when tracer.waitSplit is true and waitKeyspaceRegionSplit returns an error (including failpoint injection), log the failure with context (keep the existing zap fields) but do not return nil,error — instead continue and return the created keyspace result (i.e., do not treat post-commit split-wait failures as transaction failures). Update the branch around tracer.waitSplit / manager.waitKeyspaceRegionSplit in CreateKeyspace so callers receive the created keyspace even if the split wait failed.pkg/keyspace/keyspace_test.go (1)
150-157:⚠️ Potential issue | 🟡 MinorThis failpoint never executes in this suite.
manageris created with&mockConfig{}inSetupTest, soToWaitRegionSplit()returnsfalse. ThewaitSplitKeyspaceFailedfailpoint at line 150 is behind a dead branch increateKeyspaceWithoutCheck(guarded byif tracer.waitSplit). This block is currently just another happy-path create and does not actually cover the post-commit failure path.Configure non-zero wait-split settings here, or move the injection to a path that always runs.
🤖 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 150 - 157, The failpoint waitSplitKeyspaceFailed never runs because manager is initialized with &mockConfig{} in SetupTest so ToWaitRegionSplit() returns false and the if tracer.waitSplit branch in createKeyspaceWithoutCheck is skipped; fix by enabling the wait-split path in this test: construct or configure manager with a non-zero wait-split setting (or replace &mockConfig{} with a config that returns true from ToWaitRegionSplit), or move the failpoint injection into a code path that always executes (e.g., before calling createKeyspaceWithoutCheck or into the happy-path create function), so that the failpoint "github.com/tikv/pd/pkg/keyspace/waitSplitKeyspaceFailed" is actually hit when calling manager.CreateKeyspace; ensure references to ToWaitRegionSplit, createKeyspaceWithoutCheck, tracer.waitSplit, and manager are used to locate and change the test setup.
🧹 Nitpick comments (1)
pkg/keyspace/keyspace.go (1)
296-305: Consider pre-allocating with exact capacity.The slices are initialized with capacity 3, but the actual number of operations depends on runtime conditions (e.g.,
skipSplitRegionfailpoint, nilkgm). This is minor, but usingmake([]txnOp, 0)or dynamically computing the expected count would be slightly cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/keyspace/keyspace.go` around lines 296 - 305, The txnOps/txnCbs slices are preallocated with capacity 3 even though the actual number depends on runtime conditions (e.g., skipSplitRegion failpoint and presence of kgm); update the initialization to either use make([]txnOp, 0) and make([]txnCb, 0) (no fixed capacity) or compute the expected capacity based on those conditions before calling make. Modify the declarations of txnOps and txnCbs and keep the addTxn closure as-is (txnOps, txnCbs, addTxn are the symbols to change), so capacity matches runtime logic instead of hardcoding 3.
🤖 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/tso_keyspace_group.go`:
- Around line 349-351: The check that returns the wrong error uses oldKG to
detect a missing group but returns ErrKeyspaceGroupExists; update the code to
return ErrKeyspaceGroupNotExists instead. Locate the branch where oldKG == nil
(in the function handling keyspace group lookup) and replace the
ErrKeyspaceGroupExists return with ErrKeyspaceGroupNotExists so the error
accurately reflects the missing group; keep all other logic unchanged.
---
Duplicate comments:
In `@pkg/keyspace/keyspace_test.go`:
- Around line 150-157: The failpoint waitSplitKeyspaceFailed never runs because
manager is initialized with &mockConfig{} in SetupTest so ToWaitRegionSplit()
returns false and the if tracer.waitSplit branch in createKeyspaceWithoutCheck
is skipped; fix by enabling the wait-split path in this test: construct or
configure manager with a non-zero wait-split setting (or replace &mockConfig{}
with a config that returns true from ToWaitRegionSplit), or move the failpoint
injection into a code path that always executes (e.g., before calling
createKeyspaceWithoutCheck or into the happy-path create function), so that the
failpoint "github.com/tikv/pd/pkg/keyspace/waitSplitKeyspaceFailed" is actually
hit when calling manager.CreateKeyspace; ensure references to ToWaitRegionSplit,
createKeyspaceWithoutCheck, tracer.waitSplit, and manager are used to locate and
change the test setup.
In `@pkg/keyspace/keyspace.go`:
- Around line 345-357: The CreateKeyspace path currently returns an error if
manager.waitKeyspaceRegionSplit(tracer.keyspaceID) fails after the transaction
has committed; change this to treat the split-wait as best-effort: when
tracer.waitSplit is true and waitKeyspaceRegionSplit returns an error (including
failpoint injection), log the failure with context (keep the existing zap
fields) but do not return nil,error — instead continue and return the created
keyspace result (i.e., do not treat post-commit split-wait failures as
transaction failures). Update the branch around tracer.waitSplit /
manager.waitKeyspaceRegionSplit in CreateKeyspace so callers receive the created
keyspace even if the split wait failed.
---
Nitpick comments:
In `@pkg/keyspace/keyspace.go`:
- Around line 296-305: The txnOps/txnCbs slices are preallocated with capacity 3
even though the actual number depends on runtime conditions (e.g.,
skipSplitRegion failpoint and presence of kgm); update the initialization to
either use make([]txnOp, 0) and make([]txnCb, 0) (no fixed capacity) or compute
the expected capacity based on those conditions before calling make. Modify the
declarations of txnOps and txnCbs and keep the addTxn closure as-is (txnOps,
txnCbs, addTxn are the symbols to change), so capacity matches runtime logic
instead of hardcoding 3.
🪄 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: 9bbe6b47-6718-470a-8a2c-12f02d150c4b
📥 Commits
Reviewing files that changed from the base of the PR and between 9c634d1 and 693a40babc63ee651a837b770ea0188d6ee35b41.
📒 Files selected for processing (4)
pkg/keyspace/keyspace.gopkg/keyspace/keyspace_test.gopkg/keyspace/metrics.gopkg/keyspace/tso_keyspace_group.go
Signed-off-by: tongjian <1045931706@qq.com>
693a40b to
4f06e65
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
pkg/keyspace/keyspace_test.go (1)
150-157:⚠️ Potential issue | 🟡 MinorThis failpoint still never executes in this suite.
SetupTest()buildssuite.managerwith&mockConfig{}, soToWaitRegionSplit()is false and thewaitSplitKeyspaceFailedinjection at Line 150 stays behind a dead branch. This is currently another happy-path create, not coverage for the post-commit wait failure path. Reconfigure the manager withWaitRegionSplit: truefor this case, and register the disable withdefer/Cleanupimmediately afterEnableso a failing assertion cannot leak the failpoint into later tests.As per coding guidelines, "Tests must handle failpoints using github.com/pingcap/failpoint."
🤖 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 150 - 157, The test currently enables the failpoint "waitSplitKeyspaceFailed" but SetupTest() constructs suite.manager with &mockConfig{} where ToWaitRegionSplit() is false, so the injected branch never executes; update the test to configure the manager to enable waiting by building it with WaitRegionSplit: true (or otherwise set mockConfig.ToWaitRegionSplit() to return true) before calling manager.CreateKeyspace/LoadKeyspace so the failpoint is exercised, and immediately register failpoint.Disable("github.com/tikv/pd/pkg/keyspace/waitSplitKeyspaceFailed") with defer or t.Cleanup right after failpoint.Enable to ensure the failpoint is always removed even if assertions fail; refer to SetupTest, mockConfig, ToWaitRegionSplit, manager.CreateKeyspace, and the "waitSplitKeyspaceFailed" failpoint when making the change.pkg/keyspace/keyspace.go (1)
349-360:⚠️ Potential issue | 🟠 MajorDon't report create failure after the transaction has already committed.
By the time this branch runs, the keyspace meta, group membership, region label rule, and post-commit cache updates are already durable. Returning
nil, errhere makes callers think create rolled back, but a retry will just hitErrKeyspaceExists. Treat the split wait as best-effort after commit, or return the created keyspace with an explicit "split pending" status.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/keyspace/keyspace.go` around lines 349 - 360, The wait-for-split branch currently calls manager.waitKeyspaceRegionSplit (guarded by tracer.waitSplit) and returns nil, err on failure which incorrectly signals the create rolled back; change this to treat the split wait as best-effort: catch errors from manager.waitKeyspaceRegionSplit, log a warning with context (keyspace.GetId()/GetName()) but do NOT return nil, err after the transaction has committed — instead continue returning the created keyspace; alternatively, if you need to surface state, set a "split pending" status/flag on the returned keyspace object before returning so callers know split is outstanding, and remove the existing early return that returns nil, err.
🤖 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/keyspace.go`:
- Around line 289-298: createKeyspaceWithoutCheck currently persists the new
KeyspaceMeta with State set to ENABLED while CheckKeyspaceState mutates a
caller-local copy when regions aren’t split, causing inconsistent views; either
persist the true post-create state (e.g., set State to DISABLED until split
readiness is confirmed and have CheckKeyspaceState update the stored
KeyspaceMeta) or add an explicit persisted readiness field (e.g., SplitReady or
Readiness) on keyspacepb.KeyspaceMeta and use that instead of flipping State in
CheckKeyspaceState; update createKeyspaceWithoutCheck, CheckKeyspaceState, and
the other affected block referenced (the code around the 519-534 area) so all
readers derive the same persisted state rather than mutating only in-memory
copies.
In `@pkg/keyspace/tso_keyspace_group.go`:
- Around line 494-519: The callback cb currently mutates the in-memory kg
fetched from m.groups and then calls m.groups[userKind].Put(kg), which can
overwrite a newer committed snapshot; instead, after acquiring m.Lock() when
err==nil, reload the committed group snapshot for userKind/groupID from
persistent storage (do not use the live m.groups entry), apply the
opAdd/opDelete change to that freshly loaded snapshot, and then call
m.groups[userKind].Put(...) with that reloaded-and-modified snapshot (or skip
Put if the reload shows the mutation already applied); reference cb, m.groups,
userKind, groupID, kg, Put, opAdd and opDelete when making the change.
---
Duplicate comments:
In `@pkg/keyspace/keyspace_test.go`:
- Around line 150-157: The test currently enables the failpoint
"waitSplitKeyspaceFailed" but SetupTest() constructs suite.manager with
&mockConfig{} where ToWaitRegionSplit() is false, so the injected branch never
executes; update the test to configure the manager to enable waiting by building
it with WaitRegionSplit: true (or otherwise set mockConfig.ToWaitRegionSplit()
to return true) before calling manager.CreateKeyspace/LoadKeyspace so the
failpoint is exercised, and immediately register
failpoint.Disable("github.com/tikv/pd/pkg/keyspace/waitSplitKeyspaceFailed")
with defer or t.Cleanup right after failpoint.Enable to ensure the failpoint is
always removed even if assertions fail; refer to SetupTest, mockConfig,
ToWaitRegionSplit, manager.CreateKeyspace, and the "waitSplitKeyspaceFailed"
failpoint when making the change.
In `@pkg/keyspace/keyspace.go`:
- Around line 349-360: The wait-for-split branch currently calls
manager.waitKeyspaceRegionSplit (guarded by tracer.waitSplit) and returns nil,
err on failure which incorrectly signals the create rolled back; change this to
treat the split wait as best-effort: catch errors from
manager.waitKeyspaceRegionSplit, log a warning with context
(keyspace.GetId()/GetName()) but do NOT return nil, err after the transaction
has committed — instead continue returning the created keyspace; alternatively,
if you need to surface state, set a "split pending" status/flag on the returned
keyspace object before returning so callers know split is outstanding, and
remove the existing early return that returns nil, err.
🪄 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: 761b7560-e073-45aa-8cfd-18c0fbdfdb76
📥 Commits
Reviewing files that changed from the base of the PR and between 693a40babc63ee651a837b770ea0188d6ee35b41 and 4f06e65.
📒 Files selected for processing (5)
pkg/keyspace/keyspace.gopkg/keyspace/keyspace_test.gopkg/keyspace/metrics.gopkg/keyspace/tso_keyspace_group.goserver/apiv2/handlers/keyspace.go
Signed-off-by: tongjian <1045931706@qq.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/keyspace/keyspace.go (2)
295-300:⚠️ Potential issue | 🟠 MajorPersisted/cached
ENABLEDstill diverges from the effective keyspace state.Line 297 persists new keyspaces as
ENABLED, Line 396 caches that value, but Line 534 only rewrites a few caller-local copies when the split is still pending. That leaves different read paths disagreeing about the same keyspace:GetKeyspaceStateByID/GetEnabledKeyspaceNameByIDwill still treat it as enabled while handlers that callCheckKeyspaceStatecan return disabled. Persist the real transition, or add a separate readiness field and use it consistently everywhere.Also applies to: 393-396, 522-537
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/keyspace/keyspace.go` around lines 295 - 300, The persisted Keyspace.State is being set to ENABLED at creation (tracer.keyspaceID/tracer.keyspaceName) but other code paths (CheckKeyspaceState) treat it as not-ready, causing divergence; fix by either (A) persisting the actual transitional state instead of always ENABLED (update the creation/site where State is assigned and where the cache is written around the caching code that references the same state), or (B) add a clear, consistent readiness field (e.g., Ready or Initialized) to the persisted Keyspace and populate it at the real transition point, then update the cache write logic and all accessors (GetKeyspaceStateByID, GetEnabledKeyspaceNameByID, CheckKeyspaceState and the cache population code) to read that single field so all read paths agree on a keyspace’s effective state.
328-345:⚠️ Potential issue | 🟠 MajorDon't surface a create failure after the transaction has committed.
By the time Line 357 runs, the metadata, keyspace-group membership, and label rule are already durable and the post-commit callbacks have executed. Returning
nil, errhere makes a successful create look like a rollback, and the next retry just turns intoErrKeyspaceExists(including the bootstrap pre-alloc retry loop above). Make the split wait best-effort after commit, or return the created keyspace with an explicit "split pending" status.Also applies to: 352-363
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/keyspace/keyspace.go` around lines 328 - 345, After the transaction commit and post-commit callbacks (the manager.store.RunInTxn block and the subsequent txnCbs iteration) you must not surface an error that would make a successful create look like a rollback; instead detect that the txn committed and convert split-wait failures into a best-effort async action or return the created keyspace with a "split pending" status. Change the error handling around err after the RunInTxn/txnCbs block so that if the metadata/membership/label updates were applied you do not return nil, err: either swallow the split/split-wait error (log it) and launch the split wait asynchronously, or populate and return the created Keyspace object with an explicit SplitPending flag; ensure callbacks already executed still see success and remove the final return of the transaction error that implies rollback (update the code paths that reference manager.store.RunInTxn, txnCbs, the err check and the log.Warn).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integrations/mcs/tso/keyspace_group_manager_test.go`:
- Around line 924-940: The test calls LoadKeyspace immediately after Bootstrap,
which can race with background pre-allocation; change the test to poll until
both keyspaces resolve before proceeding: repeatedly call
leaderServer.GetServer().GetKeyspaceManager().LoadKeyspace for each name
(keyspaces[0] and keyspaces[1]) inside a testutil.Eventually (or similar wait
loop) and only continue when both return without error, then use their Ids for
the subsequent SplitKeyspaceGroupByID, waitFinishSplit, and assertions
(references: LoadKeyspace, GetKeyspaceManager, leaderServer,
SplitKeyspaceGroupByID, waitFinishSplit).
---
Duplicate comments:
In `@pkg/keyspace/keyspace.go`:
- Around line 295-300: The persisted Keyspace.State is being set to ENABLED at
creation (tracer.keyspaceID/tracer.keyspaceName) but other code paths
(CheckKeyspaceState) treat it as not-ready, causing divergence; fix by either
(A) persisting the actual transitional state instead of always ENABLED (update
the creation/site where State is assigned and where the cache is written around
the caching code that references the same state), or (B) add a clear, consistent
readiness field (e.g., Ready or Initialized) to the persisted Keyspace and
populate it at the real transition point, then update the cache write logic and
all accessors (GetKeyspaceStateByID, GetEnabledKeyspaceNameByID,
CheckKeyspaceState and the cache population code) to read that single field so
all read paths agree on a keyspace’s effective state.
- Around line 328-345: After the transaction commit and post-commit callbacks
(the manager.store.RunInTxn block and the subsequent txnCbs iteration) you must
not surface an error that would make a successful create look like a rollback;
instead detect that the txn committed and convert split-wait failures into a
best-effort async action or return the created keyspace with a "split pending"
status. Change the error handling around err after the RunInTxn/txnCbs block so
that if the metadata/membership/label updates were applied you do not return
nil, err: either swallow the split/split-wait error (log it) and launch the
split wait asynchronously, or populate and return the created Keyspace object
with an explicit SplitPending flag; ensure callbacks already executed still see
success and remove the final return of the transaction error that implies
rollback (update the code paths that reference manager.store.RunInTxn, txnCbs,
the err check and the log.Warn).
🪄 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: b388bcb0-9876-4822-879c-66a73fbc25cd
📒 Files selected for processing (3)
pkg/keyspace/keyspace.gopkg/keyspace/tso_keyspace_group.gotests/integrations/mcs/tso/keyspace_group_manager_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/keyspace/tso_keyspace_group.go
Signed-off-by: tongjian <1045931706@qq.com>
618a771 to
252d9c8
Compare
| err2 := manager.store.RunInTxn(manager.ctx, func(txn kv.Txn) error { | ||
| metaPath := keypath.KeyspaceMetaPath(id) | ||
| return txn.Remove(metaPath) | ||
| if tracer.waitSplit { |
There was a problem hiding this comment.
Do we need rollback keyspace if split failed? Or use Disabled as keyspace default status when we created it?
This now commits the keyspace meta / keyspace-group membership / region label rule before the region split has actually finished, and the persisted keyspace state is already ENABLED at that point.
That changes the failure semantics of CreateKeyspace: if the subsequent wait-for-split step times out, CreateKeyspace returns an error, but the keyspace has already been created and committed. A retry will then immediately hit ErrKeyspaceExists. In other words, "create failed" no longer means "nothing was created".
There was a problem hiding this comment.
func TestWaitSplitFailureLeavesCommittedKeyspace(t *testing.T) {
re := require.New(t)
manager := setupMinimalKeyspaceManager(t)
manager.UpdateConfig(&mockConfig{
WaitRegionSplit: true,
WaitRegionSplitTimeout: typeutil.Duration{Duration: time.Millisecond},
CheckRegionSplitInterval: typeutil.Duration{Duration: time.Millisecond},
})
req := &CreateKeyspaceRequest{
Name: "waitsplitcase1",
CreateTime: time.Now().Unix(),
}
created, err := manager.CreateKeyspace(req)
re.Nil(created)
re.ErrorIs(err, errs.ErrRegionSplitTimeout)
loaded, err := manager.LoadKeyspace(req.Name)
re.NoError(err)
re.Equal(keyspacepb.KeyspaceState_ENABLED, loaded.State)
}
I added a minimal unit test to verify this behavior: after CreateKeyspace returns ErrRegionSplitTimeout, LoadKeyspace still succeeds and the stored state is ENABLED (see TestWaitSplitFailureLeavesCommittedKeyspace).
There was a problem hiding this comment.
We just ensure the meta is atomic; the keyspace state of the API checks the split status.
pd/server/apiv2/handlers/keyspace.go
Lines 173 to 175 in 4f06e65
There was a problem hiding this comment.
That means a retry does not retry the same create attempt semantics anymore; it just immediately hits ErrKeyspaceExists.
I think we still need to fix one of these:
- treat the post-commit wait-split as best-effort and return the created keyspace, or
- persist a real transitional state / readiness signal so the API and callers can distinguish "created but split pending" from a rollback.
There was a problem hiding this comment.
We’ll keep falling into the transaction failure path, while in fact, we only need to wait a bit longer here.
| if meta.State != keyspacepb.KeyspaceState_ENABLED { | ||
| return | ||
| } | ||
| if !manager.CheckKeyspaceRegionBound(meta.Id) { | ||
| meta.State = keyspacepb.KeyspaceState_DISABLED | ||
| return | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
It will miss some path
This read-time CheckKeyspaceState patch only fixes a few single-keyspace read paths, but it does not fix the underlying state inconsistency.
Because create now persists the keyspace as ENABLED before split completion, the same keyspace can be observed with different states depending on which path reads it:
- LoadKeyspace + CheckKeyspaceState sees DISABLED
- LoadRangeKeyspace / LoadAllKeyspaces still see ENABLED
- background logic such as GC also still treats it as ENABLED
There was a problem hiding this comment.
func TestLoadRangeKeyspaceStillShowsEnabledBeforeSplitComplete(t *testing.T) {
re := require.New(t)
manager := setupMinimalKeyspaceManager(t)
req := &CreateKeyspaceRequest{
Name: "rangebefore1",
CreateTime: time.Now().Unix(),
}
created, err := manager.CreateKeyspace(req)
re.NoError(err)
single, err := manager.LoadKeyspace(req.Name)
re.NoError(err)
manager.CheckKeyspaceState(single)
re.Equal(keyspacepb.KeyspaceState_DISABLED, single.State)
scanned, err := manager.LoadRangeKeyspace(created.Id, 1)
re.NoError(err)
re.Len(scanned, 1)
re.Equal(created.Id, scanned[0].Id)
re.Equal(keyspacepb.KeyspaceState_ENABLED, scanned[0].State)
}
There was a problem hiding this comment.
yes, LoadAllKeyspaces is too big; checking all keyspaces' split status is expensive.
There was a problem hiding this comment.
My concern is not only LoadAllKeyspaces.
Right now the same keyspace can still be observed with different effective states depending on the read path:
- detail API paths call
CheckKeyspaceState LoadRangeKeyspace/ list APIs still expose the persistedENABLEDGetKeyspaceStateByID/GetEnabledKeyspaceNameByIDstill read the cached/persisted state directly- background logic also still consumes that state, for example GC and storage-size collection paths
| userKind := endpoint.StringUserKind(request.Config[UserKindKey]) | ||
| config, err := manager.kgm.GetKeyspaceConfigByKind(userKind) | ||
| addTxn(op, cb) | ||
| groupID, err := strconv.ParseUint(config[TSOKeyspaceGroupIDKey], 10, 64) |
There was a problem hiding this comment.
This assumes TSOKeyspaceGroupIDKey is always present, but that is not true when keyspace-group is disabled.
CI is already hitting strconv.ParseUint: parsing "": invalid syntax during keyspace manager bootstrap.
bufferflies
changed the title
Reconcile the atomic keyspace-creation rewrite with two features added on master since this branch diverged: - meta-service groups: fold group selection and the persisted assignment-count increment into the same creation transaction (assignMetaServiceGroupTxnOp) instead of master's separate pick + save + manual rollback, so a failed creation never leaks an assignment count. The mgm read lock is held for the whole txn to stay race-free with group deletion. - region bound type (raw/txn): thread boundType through the label-rule txn op, the split wait, and CheckKeyspaceRegionBound (kept on master's meta-based signature used by keyspace_service.go). Keep master's RemoveKeyspacesFromGroup and re-add the externalAllocNode failpoint inside updateKeyspaceForGroupTxnOp. Drop the now-unused UpdateKeyspaceForGroup/assignGroupAndSaveKeyspace/splitKeyspaceRegion paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
The test invoked the post-commit callback while still holding the meta-service group manager's read lock. The callback re-acquires that lock via AttachEndpoints, which the deadlock detector flags as a recursive RLock. Match the real create path: run the assignment op under the lock, release it, then run the callback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
In classic mode the keyspace group manager is nil, so
GetKeyspaceConfigByKind returns an empty config without a TSO keyspace
group id. createKeyspaceWithoutCheck parsed that id unconditionally, so
strconv.ParseUint("") failed and bootstrapping the keyspace manager (and
every keyspace creation) errored out, cascading into GC / keyspace /
region-label / resource-manager / TSO integration test failures.
Only parse the TSO keyspace group id when present and fall back to the
default keyspace group id (0), which is only used to serialize the
creation transaction. The keyspace-group txn op is already a no-op when
the manager is nil.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: tongjian <1045931706@qq.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10492 +/- ##
==========================================
+ Coverage 79.30% 79.31% +0.01%
==========================================
Files 541 541
Lines 75847 75917 +70
==========================================
+ Hits 60148 60216 +68
+ Misses 11475 11453 -22
- Partials 4224 4248 +24
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
5e6e276 to
40339b1
Compare
The keyspace is persisted as ENABLED before its region split completes, which surfaced two review concerns: - The apiv2 handlers called CheckKeyspaceState, which rewrote ENABLED to DISABLED at read time only on the single-keyspace API paths, so the same keyspace was observed with different states depending on the read path (LoadRangeKeyspace and GC still saw ENABLED). Remove CheckKeyspaceState so every path reads the single authoritative persisted state; clients are still prevented from using a not-yet-split keyspace by the gRPC CheckKeyspaceRegionBound guard. - A post-commit wait-for-split failure returned an error even though the keyspace was already committed atomically, so a retry would only hit ErrKeyspaceExists. Treat the wait as best-effort: log and return the created keyspace; patrol will split the region. Add TestWaitSplitFailureStillCreatesKeyspace and drop leftover debug prints in the pd-ctl keyspace test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: tongjian <1045931706@qq.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/keyspace/tso_keyspace_group.go (1)
557-597: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winParse the group ID as
uint32before using it.
ParseUint(..., 64)followed byuint32(groupID)can truncate out-of-range IDs and point at the wrong keyspace group. Parse with bit size32once, then reuse the checked value.Proposed fix
- groupID, err := strconv.ParseUint(id, 10, 64) + parsedGroupID, err := strconv.ParseUint(id, 10, 32) if err != nil { return nil, nil, err } + groupID := uint32(parsedGroupID) ... - kg := m.groups[userKind].Get(uint32(groupID)) + kg := m.groups[userKind].Get(groupID) ... - return nil, nil, errs.ErrKeyspaceGroupNotExists.FastGenByArgs(uint32(groupID)) + return nil, nil, errs.ErrKeyspaceGroupNotExists.FastGenByArgs(groupID) ... - kg, loadErr = m.store.LoadKeyspaceGroup(txn, uint32(groupID)) + kg, loadErr = m.store.LoadKeyspaceGroup(txn, groupID) ... - op := m.saveKeyspaceGroupTxnOp(uint32(groupID), keyspaceID, mutation) + op := m.saveKeyspaceGroupTxnOp(groupID, keyspaceID, mutation)🤖 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/keyspace/tso_keyspace_group.go` around lines 557 - 597, Parse and validate the keyspace group ID as uint32 up front in the group-loading path so it cannot be truncated later; in the logic around strconv.ParseUint, m.groups[userKind].Get, and saveKeyspaceGroupTxnOp, change the parse/check flow to use a 32-bit bound once and reuse that validated value everywhere. Avoid converting a 64-bit parsed value back to uint32 at each use site, and keep the existing not-found/splitting/merging checks operating on the checked uint32 group ID.Source: Linters/SAST tools
🧹 Nitpick comments (1)
pkg/keyspace/keyspace.go (1)
199-203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
errors.IsforErrKeyspaceExists.
A wrapped sentinel will miss this check here and in the matchinginitReserveKeyspacepath below.Proposed fix
- if err != nil && err != errs.ErrKeyspaceExists { + if err != nil && !errors.Is(err, errs.ErrKeyspaceExists) {🤖 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/keyspace/keyspace.go` around lines 199 - 203, The ErrKeyspaceExists check in createKeyspace and the matching initReserveKeyspace path should use errors.Is instead of direct comparison, since wrapped errors will otherwise be missed. Update the conditional around manager.CreateKeyspace(req) and the corresponding sentinel check below to call errors.Is(err, errs.ErrKeyspaceExists) while keeping the existing retry/logging behavior for all other errors.Source: Coding guidelines
🤖 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/keyspace/keyspace.go`:
- Around line 388-394: The keyspace group ID parsing in keyspace.go allows
values that fit in uint64 but overflow when cast to uint32 before
runCreateKeyspaceTxn, which can use the wrong lock key. Update the
parsing/validation in the code path that reads TSOKeyspaceGroupIDKey so it
rejects any value above uint32 before the cast, either by parsing with 32-bit
bounds or by adding an explicit range check before calling
manager.runCreateKeyspaceTxn.
- Around line 953-966: The keyspace removal flow in the manager currently
deletes `keyspaceNameLookup` and `keyspaceStateLookup` before the fallible
`updateAssignmentTxn` call, so a transaction failure leaves caches out of sync.
In the `RemoveKeyspace` path, move the cache eviction for
`manager.keyspaceNameLookup.Delete(id)` and
`manager.keyspaceStateLookup.Delete(id)` to after all transaction operations
that can fail, or defer them until after commit if that hook exists. Keep the
existing `manager.store.RemoveKeyspace` and `manager.mgm.updateAssignmentTxn`
checks as the gating points, and only evict once the txn work is guaranteed to
succeed.
In `@pkg/keyspace/tso_keyspace_group.go`:
- Around line 526-535: The keyspace removal in the keyspace group transaction is
clearing caches too early. Update the flow around `km.RemoveKeyspace` in
`tso_keyspace_group.go` so the transaction only performs the storage-side
removal while `keyspaceNameLookup` and `keyspaceStateLookup` are left intact
until `RunInTxn` returns successfully. After `SaveKeyspaceGroup` commits
successfully, then delete the caches using the existing keyspace manager
helpers.
- Around line 561-565: The failpoint setup in
failpoint.Inject("externalAllocNode") is ignoring the error returned by
SetNodesForKeyspaceGroup, which can mask a broken configuration. Update the
closure to capture the result of m.SetNodesForKeyspaceGroup in
tso_keyspace_group.go and handle any failure explicitly, for example by logging
or otherwise surfacing the error instead of assigning it to _. Keep the fix
localized to the externalAllocNode failpoint block and preserve failpointOnce.Do
behavior.
---
Outside diff comments:
In `@pkg/keyspace/tso_keyspace_group.go`:
- Around line 557-597: Parse and validate the keyspace group ID as uint32 up
front in the group-loading path so it cannot be truncated later; in the logic
around strconv.ParseUint, m.groups[userKind].Get, and saveKeyspaceGroupTxnOp,
change the parse/check flow to use a 32-bit bound once and reuse that validated
value everywhere. Avoid converting a 64-bit parsed value back to uint32 at each
use site, and keep the existing not-found/splitting/merging checks operating on
the checked uint32 group ID.
---
Nitpick comments:
In `@pkg/keyspace/keyspace.go`:
- Around line 199-203: The ErrKeyspaceExists check in createKeyspace and the
matching initReserveKeyspace path should use errors.Is instead of direct
comparison, since wrapped errors will otherwise be missed. Update the
conditional around manager.CreateKeyspace(req) and the corresponding sentinel
check below to call errors.Is(err, errs.ErrKeyspaceExists) while keeping the
existing retry/logging behavior for all other errors.
🪄 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: 27a5be91-5eb2-4dc9-970d-0f357953b774
📒 Files selected for processing (7)
pkg/keyspace/keyspace.gopkg/keyspace/keyspace_test.gopkg/keyspace/tso_keyspace_group.gopkg/keyspace/tso_keyspace_group_test.gopkg/schedule/labeler/labeler.gotests/integrations/mcs/tso/api_test.gotests/integrations/mcs/tso/keyspace_group_manager_test.go
💤 Files with no reviewable changes (2)
- tests/integrations/mcs/tso/api_test.go
- tests/integrations/mcs/tso/keyspace_group_manager_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/schedule/labeler/labeler.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/keyspace/tso_keyspace_group.go (1)
557-597: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winParse the group ID as
uint32before using it.
ParseUint(..., 64)followed byuint32(groupID)can truncate out-of-range IDs and point at the wrong keyspace group. Parse with bit size32once, then reuse the checked value.Proposed fix
- groupID, err := strconv.ParseUint(id, 10, 64) + parsedGroupID, err := strconv.ParseUint(id, 10, 32) if err != nil { return nil, nil, err } + groupID := uint32(parsedGroupID) ... - kg := m.groups[userKind].Get(uint32(groupID)) + kg := m.groups[userKind].Get(groupID) ... - return nil, nil, errs.ErrKeyspaceGroupNotExists.FastGenByArgs(uint32(groupID)) + return nil, nil, errs.ErrKeyspaceGroupNotExists.FastGenByArgs(groupID) ... - kg, loadErr = m.store.LoadKeyspaceGroup(txn, uint32(groupID)) + kg, loadErr = m.store.LoadKeyspaceGroup(txn, groupID) ... - op := m.saveKeyspaceGroupTxnOp(uint32(groupID), keyspaceID, mutation) + op := m.saveKeyspaceGroupTxnOp(groupID, keyspaceID, mutation)🤖 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/keyspace/tso_keyspace_group.go` around lines 557 - 597, Parse and validate the keyspace group ID as uint32 up front in the group-loading path so it cannot be truncated later; in the logic around strconv.ParseUint, m.groups[userKind].Get, and saveKeyspaceGroupTxnOp, change the parse/check flow to use a 32-bit bound once and reuse that validated value everywhere. Avoid converting a 64-bit parsed value back to uint32 at each use site, and keep the existing not-found/splitting/merging checks operating on the checked uint32 group ID.Source: Linters/SAST tools
🧹 Nitpick comments (1)
pkg/keyspace/keyspace.go (1)
199-203: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
errors.IsforErrKeyspaceExists.
A wrapped sentinel will miss this check here and in the matchinginitReserveKeyspacepath below.Proposed fix
- if err != nil && err != errs.ErrKeyspaceExists { + if err != nil && !errors.Is(err, errs.ErrKeyspaceExists) {🤖 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/keyspace/keyspace.go` around lines 199 - 203, The ErrKeyspaceExists check in createKeyspace and the matching initReserveKeyspace path should use errors.Is instead of direct comparison, since wrapped errors will otherwise be missed. Update the conditional around manager.CreateKeyspace(req) and the corresponding sentinel check below to call errors.Is(err, errs.ErrKeyspaceExists) while keeping the existing retry/logging behavior for all other errors.Source: Coding guidelines
🤖 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/keyspace/keyspace.go`:
- Around line 388-394: The keyspace group ID parsing in keyspace.go allows
values that fit in uint64 but overflow when cast to uint32 before
runCreateKeyspaceTxn, which can use the wrong lock key. Update the
parsing/validation in the code path that reads TSOKeyspaceGroupIDKey so it
rejects any value above uint32 before the cast, either by parsing with 32-bit
bounds or by adding an explicit range check before calling
manager.runCreateKeyspaceTxn.
- Around line 953-966: The keyspace removal flow in the manager currently
deletes `keyspaceNameLookup` and `keyspaceStateLookup` before the fallible
`updateAssignmentTxn` call, so a transaction failure leaves caches out of sync.
In the `RemoveKeyspace` path, move the cache eviction for
`manager.keyspaceNameLookup.Delete(id)` and
`manager.keyspaceStateLookup.Delete(id)` to after all transaction operations
that can fail, or defer them until after commit if that hook exists. Keep the
existing `manager.store.RemoveKeyspace` and `manager.mgm.updateAssignmentTxn`
checks as the gating points, and only evict once the txn work is guaranteed to
succeed.
In `@pkg/keyspace/tso_keyspace_group.go`:
- Around line 526-535: The keyspace removal in the keyspace group transaction is
clearing caches too early. Update the flow around `km.RemoveKeyspace` in
`tso_keyspace_group.go` so the transaction only performs the storage-side
removal while `keyspaceNameLookup` and `keyspaceStateLookup` are left intact
until `RunInTxn` returns successfully. After `SaveKeyspaceGroup` commits
successfully, then delete the caches using the existing keyspace manager
helpers.
- Around line 561-565: The failpoint setup in
failpoint.Inject("externalAllocNode") is ignoring the error returned by
SetNodesForKeyspaceGroup, which can mask a broken configuration. Update the
closure to capture the result of m.SetNodesForKeyspaceGroup in
tso_keyspace_group.go and handle any failure explicitly, for example by logging
or otherwise surfacing the error instead of assigning it to _. Keep the fix
localized to the externalAllocNode failpoint block and preserve failpointOnce.Do
behavior.
---
Outside diff comments:
In `@pkg/keyspace/tso_keyspace_group.go`:
- Around line 557-597: Parse and validate the keyspace group ID as uint32 up
front in the group-loading path so it cannot be truncated later; in the logic
around strconv.ParseUint, m.groups[userKind].Get, and saveKeyspaceGroupTxnOp,
change the parse/check flow to use a 32-bit bound once and reuse that validated
value everywhere. Avoid converting a 64-bit parsed value back to uint32 at each
use site, and keep the existing not-found/splitting/merging checks operating on
the checked uint32 group ID.
---
Nitpick comments:
In `@pkg/keyspace/keyspace.go`:
- Around line 199-203: The ErrKeyspaceExists check in createKeyspace and the
matching initReserveKeyspace path should use errors.Is instead of direct
comparison, since wrapped errors will otherwise be missed. Update the
conditional around manager.CreateKeyspace(req) and the corresponding sentinel
check below to call errors.Is(err, errs.ErrKeyspaceExists) while keeping the
existing retry/logging behavior for all other errors.
🪄 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: 27a5be91-5eb2-4dc9-970d-0f357953b774
📒 Files selected for processing (7)
pkg/keyspace/keyspace.gopkg/keyspace/keyspace_test.gopkg/keyspace/tso_keyspace_group.gopkg/keyspace/tso_keyspace_group_test.gopkg/schedule/labeler/labeler.gotests/integrations/mcs/tso/api_test.gotests/integrations/mcs/tso/keyspace_group_manager_test.go
💤 Files with no reviewable changes (2)
- tests/integrations/mcs/tso/api_test.go
- tests/integrations/mcs/tso/keyspace_group_manager_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/schedule/labeler/labeler.go
🛑 Comments failed to post (4)
pkg/keyspace/keyspace.go (2)
388-394: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Find similar ParseUint-to-uint32 narrowing patterns. rg -n -C 3 'ParseUint\(.*,\s*10,\s*64\).*|uint32\(groupID\)' pkg/keyspace/keyspace.goRepository: tikv/pd
Length of output: 564
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the surrounding implementation and all uses of the keyspace group ID. sed -n '340,430p' pkg/keyspace/keyspace.go printf '\n---\n' rg -n -C 2 'TSOKeyspaceGroupIDKey|groupID|runCreateKeyspaceTxn|ParseUint\(' pkg/keyspace -g '*.go' printf '\n---\n' # Look for the type/validation of keyspace group IDs elsewhere in the repo. rg -n -C 2 'uint32\(.*groupID|keyspace group id|group id|KeyspaceGroupID' . -g '*.go'Repository: tikv/pd
Length of output: 50364
Reject values above
uint32before locking.ParseUint(..., 64)accepts IDs that wrap when cast touint32, so a large config value can serialize the txn under the wrong lock key. Parse with 32 bits or add an explicit range check beforerunCreateKeyspaceTxn.🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 393-393: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint32(groupID)
Note: [CWE-190] Integer Overflow or Wraparound.(integer-overflow-narrowing-conversion-go)
🤖 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/keyspace/keyspace.go` around lines 388 - 394, The keyspace group ID parsing in keyspace.go allows values that fit in uint64 but overflow when cast to uint32 before runCreateKeyspaceTxn, which can use the wrong lock key. Update the parsing/validation in the code path that reads TSOKeyspaceGroupIDKey so it rejects any value above uint32 before the cast, either by parsing with 32-bit bounds or by adding an explicit range check before calling manager.runCreateKeyspaceTxn.Source: Linters/SAST tools
953-966: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don’t evict caches before the assignment update can fail.
If
updateAssignmentTxnreturns an error, the transaction aborts butkeyspaceNameLookupandkeyspaceStateLookuphave already been deleted. Move cache eviction after all fallible txn ops at minimum; ideally defer it to a post-commit callback.Minimal fix
err = manager.store.RemoveKeyspace(txn, id, meta.GetName()) if err != nil { return err } - manager.keyspaceNameLookup.Delete(id) - manager.keyspaceStateLookup.Delete(id) // Decrement the meta-service group assignment count in the same txn so the // persisted counter stays in sync with the keyspaces actually referencing the // group. Without this, removed keyspaces leak count and could permanently // block deleting an otherwise-empty group. if manager.mgm != nil { if groupID := meta.GetConfig()[MetaServiceGroupIDKey]; groupID != "" { if err := manager.mgm.updateAssignmentTxn(txn, groupID, ""); err != nil { return err } } } + manager.keyspaceNameLookup.Delete(id) + manager.keyspaceStateLookup.Delete(id) return nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.err = manager.store.RemoveKeyspace(txn, id, meta.GetName()) if err != nil { return err } // Decrement the meta-service group assignment count in the same txn so the // persisted counter stays in sync with the keyspaces actually referencing the // group. Without this, removed keyspaces leak count and could permanently // block deleting an otherwise-empty group. if manager.mgm != nil { if groupID := meta.GetConfig()[MetaServiceGroupIDKey]; groupID != "" { if err := manager.mgm.updateAssignmentTxn(txn, groupID, ""); err != nil { return err } } } manager.keyspaceNameLookup.Delete(id) manager.keyspaceStateLookup.Delete(id)🤖 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/keyspace/keyspace.go` around lines 953 - 966, The keyspace removal flow in the manager currently deletes `keyspaceNameLookup` and `keyspaceStateLookup` before the fallible `updateAssignmentTxn` call, so a transaction failure leaves caches out of sync. In the `RemoveKeyspace` path, move the cache eviction for `manager.keyspaceNameLookup.Delete(id)` and `manager.keyspaceStateLookup.Delete(id)` to after all transaction operations that can fail, or defer them until after commit if that hook exists. Keep the existing `manager.store.RemoveKeyspace` and `manager.mgm.updateAssignmentTxn` checks as the gating points, and only evict once the txn work is guaranteed to succeed.pkg/keyspace/tso_keyspace_group.go (2)
526-535: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Defer keyspace cache deletion until after the transaction commits.
Line 526 calls
km.RemoveKeyspaceinside the transaction, but that helper deleteskeyspaceNameLookup/keyspaceStateLookupbeforeSaveKeyspaceGroupand commit complete. If the group save or commit fails, storage rolls back while the keyspace cache is already cleared. Use a storage-only removal helper in the txn and delete the caches only afterRunInTxnsucceeds.🤖 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/keyspace/tso_keyspace_group.go` around lines 526 - 535, The keyspace removal in the keyspace group transaction is clearing caches too early. Update the flow around `km.RemoveKeyspace` in `tso_keyspace_group.go` so the transaction only performs the storage-side removal while `keyspaceNameLookup` and `keyspaceStateLookup` are left intact until `RunInTxn` returns successfully. After `SaveKeyspaceGroup` commits successfully, then delete the caches using the existing keyspace manager helpers.
561-565: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle the failpoint setup error.
Silently discarding
SetNodesForKeyspaceGroupfailures can hide a broken failpoint setup.Proposed fix
+ var failpointErr error failpoint.Inject("externalAllocNode", func(val failpoint.Value) { failpointOnce.Do(func() { addrs := val.(string) - _ = m.SetNodesForKeyspaceGroup(constant.DefaultKeyspaceGroupID, strings.Split(addrs, ",")) + failpointErr = m.SetNodesForKeyspaceGroup(constant.DefaultKeyspaceGroupID, strings.Split(addrs, ",")) }) }) + if failpointErr != nil { + return nil, nil, failpointErr + }As per coding guidelines, “No ignored errors unless allowed in
.golangci.ymlerrcheck exclusions.”📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.var failpointErr error failpoint.Inject("externalAllocNode", func(val failpoint.Value) { failpointOnce.Do(func() { addrs := val.(string) failpointErr = m.SetNodesForKeyspaceGroup(constant.DefaultKeyspaceGroupID, strings.Split(addrs, ",")) }) }) if failpointErr != nil { return nil, nil, failpointErr }🤖 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/keyspace/tso_keyspace_group.go` around lines 561 - 565, The failpoint setup in failpoint.Inject("externalAllocNode") is ignoring the error returned by SetNodesForKeyspaceGroup, which can mask a broken configuration. Update the closure to capture the result of m.SetNodesForKeyspaceGroup in tso_keyspace_group.go and handle any failure explicitly, for example by logging or otherwise surfacing the error instead of assigning it to _. Keep the fix localized to the externalAllocNode failpoint block and preserve failpointOnce.Do behavior.Source: Coding guidelines
| tracer.OnGetConfigFinished() | ||
| now := time.Now().Unix() | ||
| _, err = manager.createKeyspaceWithoutCheck(tracer, config, now) | ||
| if err != nil && err != errs.ErrKeyspaceExists { |
There was a problem hiding this comment.
Ignoring ErrKeyspaceExists here makes reserved-keyspace bootstrap non-failing, but it also removes the repair behavior. Once saveNewKeyspaceTxnOp returns ErrKeyspaceExists, createKeyspaceWithoutCheck stops running the later txn ops, so the keyspace-group membership and region label rule are not re-upserted. The previous flow still set the region label and then updated the keyspace group even when the keyspace meta already existed. If restart/recovery finds the reserved keyspace meta but the membership or label rule is missing, Bootstrap now silently succeeds with incomplete state. Please explicitly ensure the reserved keyspace membership and label rule on the ErrKeyspaceExists path, or split those steps into idempotent repair operations.
| return errs.ErrKeyspaceGroupNotExists.FastGenByArgs(uint32(groupID)) | ||
| return nil, nil, errs.ErrKeyspaceGroupNotExists.FastGenByArgs(uint32(groupID)) | ||
| } | ||
| if kg.IsSplitting() { |
There was a problem hiding this comment.
This reads kg.IsSplitting() / kg.IsMerging() after taking kg from m.groups and releasing m.Lock. kg is a shared cached object, while other split/merge/update paths replace or mutate group state under m.Lock. Reading that pointer outside the lock breaks the GroupManager locking convention and can become a race. The returned txn op already reloads the group from storage and validates split/merge state, so please either keep the cache precheck fully under the lock or remove this unlocked precheck and rely on the transactional validation.
| kg, loadErr = m.store.LoadKeyspaceGroup(txn, uint32(groupID)) | ||
| return loadErr | ||
| }) | ||
| if loadErr != nil || kg == nil { |
There was a problem hiding this comment.
This post-commit callback refreshes m.groups after the transaction has already committed, but it silently returns when the reload fails or returns nil. That leaves storage with the new keyspace-group membership while the in-memory cache can remain stale, and later GetKeyspaceConfigByKind / assignment decisions may keep using the stale cache. At minimum this should log a warning; ideally it should have a recovery path such as retrying, invalidating the cache, or forcing a later reload from storage.
Signed-off-by: tongjian <1045931706@qq.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/keyspace/meta_service_group.go (2)
244-287: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
loadGroupStatuscan return nil — consider a defensive guard.
loadGroupStatusreturnsstatusMap[groupID], which isnilif the group has no persisted status. Callers inupdateAssignmentTxn(lines 259, 271) dereferencestatus.AssignmentCountwithout a nil check. Current callers are safe because they validate group existence in the same transaction snapshot before reaching this point, but a future caller that skips validation would hit an NPE. Adding a nil check with a clear error would make the helper robust.🛡️ Optional defensive guard
func (m *MetaServiceGroupManager) loadGroupStatus(txn kv.Txn, groupID string) (*endpoint.MetaServiceGroupStatus, error) { statusMap, err := m.store.LoadMetaServiceGroupStatus(txn, map[string]string{groupID: ""}) if err != nil { return nil, err } - return statusMap[groupID], nil + status := statusMap[groupID] + if status == nil { + return nil, fmt.Errorf("meta-service group %s not found", groupID) + } + return status, nil }🤖 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/keyspace/meta_service_group.go` around lines 244 - 287, `loadGroupStatus` can return nil, and `updateAssignmentTxn` currently dereferences `status.AssignmentCount` without guarding against that case. Add a defensive nil check in `updateAssignmentTxn` after calling `loadGroupStatus` for both oldGroupID and newGroupID, and return a clear error if the persisted status is missing; keep the fix localized to `MetaServiceGroupManager.updateAssignmentTxn` and the `loadGroupStatus` helper.
202-214: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid redundant full status load in
AssignToGroup.
findMinMetaGroupalready loads all group statuses viaLoadMetaServiceGroupStatus(txn, m.metaServiceGroups). Line 208 repeats the exact same full load. SinceloadGroupStatusexists for loading a single group's status, use it here to avoid the redundant full-map read.♻️ Proposed refactor
if err := m.store.RunInTxn(ctx, func(txn kv.Txn) error { var err error assignedGroup, err = m.findMinMetaGroup(txn) if err != nil { return err } - statusMap, err := m.store.LoadMetaServiceGroupStatus(txn, m.metaServiceGroups) + status, err := m.loadGroupStatus(txn, assignedGroup) if err != nil { return err } - status := statusMap[assignedGroup] status.AssignmentCount += count return m.store.SaveMetaServiceGroupStatus(txn, assignedGroup, status) }); err != nil {🤖 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/keyspace/meta_service_group.go` around lines 202 - 214, `AssignToGroup` is redundantly loading the full meta service group status map after `findMinMetaGroup` has already done so. Update the transaction block in `AssignToGroup` to reuse the selected `assignedGroup` and fetch only that group’s status with `loadGroupStatus` instead of calling `LoadMetaServiceGroupStatus` again, then increment `AssignmentCount` and persist via `SaveMetaServiceGroupStatus`.pkg/keyspace/tso_keyspace_group_test.go (1)
789-792: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
buildSequentialKeyspaceGroupIDhelper instead of manual loop.Lines 789-792 duplicate the logic already encapsulated in
buildSequentialKeyspaceGroupID(0, defaultKeyspaceCountSplitThreshold+1)defined at lines 769-775. Prefer the helper for consistency with the other patrol tests.♻️ Proposed refactor
- keyspaces := make([]uint32, 0, defaultKeyspaceCountSplitThreshold+1) - for i := 0; i <= defaultKeyspaceCountSplitThreshold; i++ { - keyspaces = append(keyspaces, uint32(i)) - } + keyspaces := buildSequentialKeyspaces(0, defaultKeyspaceCountSplitThreshold+1)🤖 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/keyspace/tso_keyspace_group_test.go` around lines 789 - 792, Replace the manual keyspace slice निर्माण in the test with the existing buildSequentialKeyspaceGroupID helper to keep the patrol tests consistent and avoid duplicated loop logic. In the test that currently initializes keyspaces with a for-loop, call buildSequentialKeyspaceGroupID(0, defaultKeyspaceCountSplitThreshold+1) instead, and use that returned value directly wherever the generated keyspace IDs are needed.
🤖 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.
Nitpick comments:
In `@pkg/keyspace/meta_service_group.go`:
- Around line 244-287: `loadGroupStatus` can return nil, and
`updateAssignmentTxn` currently dereferences `status.AssignmentCount` without
guarding against that case. Add a defensive nil check in `updateAssignmentTxn`
after calling `loadGroupStatus` for both oldGroupID and newGroupID, and return a
clear error if the persisted status is missing; keep the fix localized to
`MetaServiceGroupManager.updateAssignmentTxn` and the `loadGroupStatus` helper.
- Around line 202-214: `AssignToGroup` is redundantly loading the full meta
service group status map after `findMinMetaGroup` has already done so. Update
the transaction block in `AssignToGroup` to reuse the selected `assignedGroup`
and fetch only that group’s status with `loadGroupStatus` instead of calling
`LoadMetaServiceGroupStatus` again, then increment `AssignmentCount` and persist
via `SaveMetaServiceGroupStatus`.
In `@pkg/keyspace/tso_keyspace_group_test.go`:
- Around line 789-792: Replace the manual keyspace slice निर्माण in the test
with the existing buildSequentialKeyspaceGroupID helper to keep the patrol tests
consistent and avoid duplicated loop logic. In the test that currently
initializes keyspaces with a for-loop, call buildSequentialKeyspaceGroupID(0,
defaultKeyspaceCountSplitThreshold+1) instead, and use that returned value
directly wherever the generated keyspace IDs are needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1280e18b-cc27-44fe-8a05-d3d193391930
📒 Files selected for processing (5)
pkg/keyspace/keyspace.gopkg/keyspace/keyspace_test.gopkg/keyspace/meta_service_group.gopkg/keyspace/tso_keyspace_group.gopkg/keyspace/tso_keyspace_group_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/keyspace/tso_keyspace_group.go
- pkg/keyspace/keyspace_test.go
- pkg/keyspace/keyspace.go
Signed-off-by: tongjian <1045931706@qq.com>
| }) | ||
| if err2 != nil { | ||
| log.Warn("[create-keyspace] failed to remove pre-created keyspace after split failed", | ||
| if err != nil { |
There was a problem hiding this comment.
After this change, a wait-split failure returns success but leaves the keyspace persisted as DISABLED. I do not see any later patrol/recovery path that calls enableNewKeyspace or otherwise transitions this keyspace to ENABLED once the region is eventually split. Since retrying CreateKeyspace will only hit ErrKeyspaceExists, a transient wait timeout can leave the keyspace permanently disabled. Please either add the promised recovery/enabling path with coverage, or avoid turning a post-commit wait failure into a stuck durable state.
| keyspace.State = keyspacepb.KeyspaceState_ENABLED | ||
| _, err = manager.UpdateKeyspaceStateByID(newID, keyspacepb.KeyspaceState_ENABLED, request.CreateTime) | ||
| tracer.OnSplitRegionFinished() | ||
| keyspace, err = manager.enableNewKeyspace(tracer.keyspaceID, createTime) |
There was a problem hiding this comment.
This reintroduces the "created but reported as failed" failure mode. By the time enableNewKeyspace runs, the keyspace meta, keyspace-group membership, and region label rule have already been committed. If this second transaction fails, CreateKeyspace returns nil, err even though the keyspace already exists; retrying CreateKeyspace will only hit ErrKeyspaceExists, and the keyspace may remain DISABLED. Please make this failure mode recoverable/observable, for example by returning the created DISABLED keyspace and ensuring an idempotent enable retry path, or by folding enable into a proper recoverable state machine.
| }) | ||
| if err2 != nil { | ||
| log.Warn("[create-keyspace] failed to remove pre-created keyspace after split failed", | ||
| if err != nil { |
There was a problem hiding this comment.
This returns success while the keyspace stays DISABLED. Please add a recovery path to enable it after the split completes, or keep this as an error.
| keyspace.State = keyspacepb.KeyspaceState_ENABLED | ||
| _, err = manager.UpdateKeyspaceStateByID(newID, keyspacepb.KeyspaceState_ENABLED, request.CreateTime) | ||
| tracer.OnSplitRegionFinished() | ||
| keyspace, err = manager.enableNewKeyspace(tracer.keyspaceID, createTime) |
There was a problem hiding this comment.
This is still a post-commit failure point. If enabling fails, the keyspace already exists but CreateKeyspace returns an error and retries hit ErrKeyspaceExists.
|
|
||
| func (m *GroupManager) updateKeyspaceForGroupLocked(userKind endpoint.UserKind, groupID uint64, keyspaceID uint32, mutation int) error { | ||
| kg := m.groups[userKind].Get(uint32(groupID)) | ||
| m.Unlock() |
There was a problem hiding this comment.
Please keep the split/merge checks under the group lock, or copy the group first. Reading the cached group after unlock can race with group updates.
3a44463 to
da296f6
Compare
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 (1)
pkg/keyspace/tso_keyspace_group.go (1)
691-713: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject group IDs that exceed the uint32 contract.
strconv.ParseUint(..., 64)accepts values larger thanmath.MaxUint32, but the subsequentuint32(groupID)conversion wraps them and can cause an update intended for one ID to mutate a different keyspace group. Parse with a 32-bit limit or explicitly bounds-check before conversion.Suggested fix
- groupID, err := strconv.ParseUint(id, 10, 64) + groupID, err := strconv.ParseUint(id, 10, 32)🤖 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/keyspace/tso_keyspace_group.go` around lines 691 - 713, Update the group ID parsing before the lookup in the surrounding method to enforce the uint32 contract, either by parsing with a 32-bit size or validating against math.MaxUint32 before any uint32(groupID) conversion. Preserve the existing invalid-input error path and ensure oversized IDs are rejected rather than wrapped to another keyspace group.Source: Linters/SAST tools
🤖 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/keyspace/tso_keyspace_group.go`:
- Around line 691-713: Update the group ID parsing before the lookup in the
surrounding method to enforce the uint32 contract, either by parsing with a
32-bit size or validating against math.MaxUint32 before any uint32(groupID)
conversion. Preserve the existing invalid-input error path and ensure oversized
IDs are rejected rather than wrapped to another keyspace group.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e134409-838a-43c8-8e8b-2b95f43ca59c
📒 Files selected for processing (2)
pkg/keyspace/keyspace.gopkg/keyspace/tso_keyspace_group.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/keyspace/keyspace.go
Signed-off-by: bufferflies <1045931706@qq.com>
da296f6 to
871f0d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/keyspace/tso_keyspace_group.go (1)
730-730: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix typographical error in the log message.
There is an unintended double space in
"failed from".🧹 Proposed fix
- log.Warn("load keyspace group failed from storage", zap.Uint64("group-id", groupID), zap.Error(loadErr)) + log.Warn("load keyspace group failed from storage", zap.Uint64("group-id", groupID), zap.Error(loadErr))🤖 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/keyspace/tso_keyspace_group.go` at line 730, Correct the log message in the keyspace group loading path by changing the unintended double space in “failed from” to a single space, while preserving the existing log fields and error handling.
🤖 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/keyspace/tso_keyspace_group.go`:
- Around line 704-716: Update the groupID parsing in the relevant keyspace-group
method from strconv.ParseUint with a 64-bit size to a 32-bit size, so values
above math.MaxUint32 are rejected during parsing. Keep the existing group
validation and error handling unchanged.
---
Nitpick comments:
In `@pkg/keyspace/tso_keyspace_group.go`:
- Line 730: Correct the log message in the keyspace group loading path by
changing the unintended double space in “failed from” to a single space, while
preserving the existing log fields and error handling.
🪄 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: b12bad7a-859e-4733-895e-b41a2004eef8
📒 Files selected for processing (2)
pkg/keyspace/keyspace.gopkg/keyspace/tso_keyspace_group.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/keyspace/keyspace.go
| if kg == nil { | ||
| return errs.ErrKeyspaceGroupNotExists.FastGenByArgs(uint32(groupID)) | ||
| m.Unlock() | ||
| return nil, nil, errs.ErrKeyspaceGroupNotExists.FastGenByArgs(uint32(groupID)) | ||
| } | ||
| if kg.IsSplitting() { | ||
| return errs.ErrKeyspaceGroupInSplit.FastGenByArgs(uint32(groupID)) | ||
| m.Unlock() | ||
| return nil, nil, errs.ErrKeyspaceGroupInSplit.FastGenByArgs(uint32(groupID)) | ||
| } | ||
| if kg.IsMerging() { | ||
| return errs.ErrKeyspaceGroupInMerging.FastGenByArgs(uint32(groupID)) | ||
| m.Unlock() | ||
| return nil, nil, errs.ErrKeyspaceGroupInMerging.FastGenByArgs(uint32(groupID)) | ||
| } | ||
|
|
||
| changed := false | ||
|
|
||
| switch mutation { | ||
| case opAdd: | ||
| if !slice.Contains(kg.Keyspaces, keyspaceID) { | ||
| kg.Keyspaces = append(kg.Keyspaces, keyspaceID) | ||
| changed = true | ||
| } | ||
| case opDelete: | ||
| lenOfKeyspaces := len(kg.Keyspaces) | ||
| kg.Keyspaces = slice.Remove(kg.Keyspaces, keyspaceID) | ||
| if lenOfKeyspaces != len(kg.Keyspaces) { | ||
| changed = true | ||
| m.Unlock() |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Prevent integer overflow and truncation vulnerabilities.
The static analysis tool correctly flagged that uint32(groupID) narrows a 64-bit integer to 32 bits. Because groupID was parsed using strconv.ParseUint(id, 10, 64) earlier on line 691, an input string exceeding math.MaxUint32 (e.g., "4294967296") will succeed without a parsing error but will silently wrap around (e.g., to 0). This could allow a request to accidentally or maliciously bypass intended isolation and mutate a completely different keyspace group, such as the default group 0.
To prevent this, enforce a 32-bit limit during parsing so that out-of-range values are correctly rejected.
🛡️ Proposed fix (apply on line 691)
// Change the parsing size from 64 to 32 on line 691:
- groupID, err := strconv.ParseUint(id, 10, 64)
+ groupID, err := strconv.ParseUint(id, 10, 32)🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 705-705: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint32(groupID)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
[warning] 709-709: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint32(groupID)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
[warning] 713-713: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint32(groupID)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🤖 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/keyspace/tso_keyspace_group.go` around lines 704 - 716, Update the
groupID parsing in the relevant keyspace-group method from strconv.ParseUint
with a 64-bit size to a 32-bit size, so values above math.MaxUint32 are rejected
during parsing. Keep the existing group validation and error handling unchanged.
Source: Linters/SAST tools
| if meta == nil { | ||
| return errs.ErrKeyspaceNotFound | ||
| } | ||
| if err = manager.transformKeyspaceState(txn, meta, keyspacepb.KeyspaceState_ENABLED, now); err != nil { |
There was a problem hiding this comment.
transformKeyspaceState updates keyspaceStateLookup before SaveKeyspaceMeta and before the enclosing transaction commits. If this enable transaction fails at save or commit, the durable meta remains DISABLED while GetKeyspaceStateByID can keep returning cached ENABLED, so the post-commit cache invariant this PR is meant to establish is still broken.
Signed-off-by: bufferflies <1045931706@qq.com>
There was a problem hiding this comment.
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 `@pkg/keyspace/keyspace.go`:
- Around line 1040-1045: In enableNewKeyspace, update the transaction flow so
transformKeyspaceState runs before manager.store.SaveKeyspaceMeta(txn, meta),
ensuring the mutated enabled state is persisted. Preserve the existing error
propagation and transaction behavior, matching the ordering used by the other
keyspace update paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| err = manager.store.SaveKeyspaceMeta(txn, meta) | ||
| if err == nil { | ||
| err = manager.transformKeyspaceState(txn, meta, keyspacepb.KeyspaceState_ENABLED, now) | ||
| } | ||
|
|
||
| return err |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the implementation of transformKeyspaceState
ast-grep run --pattern $'func (manager *Manager) transformKeyspaceState($$$) {
$$$
}' --lang go pkg/keyspace/keyspace.goRepository: tikv/pd
Length of output: 145
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== file outline =='
ast-grep outline pkg/keyspace/keyspace.go --view expanded || true
echo
echo '== references to transformKeyspaceState =='
rg -n "transformKeyspaceState" pkg/keyspace/keyspace.goRepository: tikv/pd
Length of output: 6881
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1020,1105p' pkg/keyspace/keyspace.go | cat -nRepository: tikv/pd
Length of output: 4171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== transformKeyspaceState remainder =='
sed -n '1105,1145p' pkg/keyspace/keyspace.go | cat -n
echo
echo '== SaveKeyspaceMeta definition =='
rg -n "func .*SaveKeyspaceMeta|SaveKeyspaceMeta\(" -S . | sed -n '1,40p'Repository: tikv/pd
Length of output: 3893
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== OnCommit usage =='
rg -n "OnCommit\(" pkg . | sed -n '1,120p'
echo
echo '== StorageEndpoint.SaveKeyspaceMeta =='
sed -n '1,120p' pkg/storage/endpoint/keyspace.go | cat -nRepository: tikv/pd
Length of output: 166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== StorageEndpoint.SaveKeyspaceMeta implementation =='
sed -n '1,120p' pkg/storage/endpoint/keyspace.go | cat -n
echo
echo '== call sites around 928 and 1003 =='
sed -n '910,1015p' pkg/keyspace/keyspace.go | cat -nRepository: tikv/pd
Length of output: 9736
Persist the transformed keyspace meta after the state update
enableNewKeyspace saves meta before transformKeyspaceState mutates it, so the enabled state never reaches storage. Move SaveKeyspaceMeta(txn, meta) after the state transition, matching the other update paths.
🤖 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/keyspace/keyspace.go` around lines 1040 - 1045, In enableNewKeyspace,
update the transaction flow so transformKeyspaceState runs before
manager.store.SaveKeyspaceMeta(txn, meta), ensuring the mutated enabled state is
persisted. Preserve the existing error propagation and transaction behavior,
matching the ordering used by the other keyspace update paths.
Source: Learnings
Signed-off-by: bufferflies <1045931706@qq.com>
|
@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. |
4 participants
What problem does this PR solve?
Issue Number: Close #10461
What is changed and how does it work?
Check List
Tests
Code changes
Side effects
Release note
Summary by CodeRabbit