keyspace, schedule: make keyspace creation atomic #10467#10492
keyspace, schedule: make keyspace creation atomic #10467#10492bufferflies wants to merge 7 commits intotikv:masterfrom
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:
📝 WalkthroughWalkthroughConsolidates keyspace creation into a single transactional pipeline with txn-op + post-commit callbacks, replaces direct group cache/store writes with txn-op producers, updates tracer/metrics and labeler APIs, adds load-time keyspace state check, and adapts tests to the new txn-op/callback pattern. (49 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Manager as KeyspaceManager
participant Store
participant Labeler
participant GroupMgr as GroupManager
Client->>Manager: CreateKeyspace(request)
Manager->>Manager: validate name, config, optional ID
Manager->>Store: RunInTxn(txnOps: save meta/id map, region labeler rule save, group mutation)
Note right of Store: atomic commit of meta, id map, labeler, group
Store-->>Manager: txn commit success
Manager->>Labeler: post-txn callback: register/update region label rules
Manager->>GroupMgr: post-txn callback: update in-memory group membership
Manager-->>Client: return created keyspace (ENABLED)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/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
📒 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
bufferflies
force-pushed
the
bugfix/create_keyspace_atomic
branch
from
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
📒 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
| re.NoError(failpoint.Enable("github.com/tikv/pd/pkg/keyspace/waitSplitKeyspaceFailed", `return(true)`)) | ||
| ks, err := manager.CreateKeyspace(requests[0]) | ||
| re.NoError(err) | ||
| km, err := manager.LoadKeyspace(ks.Name) | ||
| re.NoError(err) | ||
| re.Equal(keyspacepb.KeyspaceState_ENABLED, km.State) | ||
|
|
||
| re.NoError(failpoint.Disable("github.com/tikv/pd/pkg/keyspace/waitSplitKeyspaceFailed")) |
There was a problem hiding this comment.
This failpoint never executes in this suite.
manager is created with &mockConfig{} in SetupTest, so ToWaitRegionSplit() is false and the waitSplitKeyspaceFailed injection at Line 143 sits behind a dead branch. This block is currently just another happy-path create, so it 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 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.
There was a problem hiding this comment.
no, injection will also affect the region labeler setting
There was a problem hiding this comment.
🧩 Analysis chain
Let me check how waitSplitKeyspaceFailed is used in the implementation.
🏁 Script executed:
#!/bin/bash
rg -n "waitSplitKeyspaceFailed" --type=go -C 10
Length of output: 3334
Let me look at more context around the waitSplitKeyspaceFailed usage and the region labeler path.
🏁 Script executed:
#!/bin/bash
# Get broader context around the saveKeyspaceRegionLabelerTxnOp and the full createKeyspaceWithoutCheck function
rg -n "saveKeyspaceRegionLabelerTxnOp\|waitSplitKeyspaceFailed\|saveKeyspaceGroupsTxnOpFailed" --type=go -A 5 -B 5
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search more broadly
rg -n "waitSplitKeyspace\|saveKeyspaceGroups\|RegionLabeler\|regionLabeler\|labeler" pkg/keyspace/keyspace.go | head -60
Length of output: 0
🏁 Script executed:
#!/bin/bash
# View the full createKeyspaceWithoutCheck function area
grep -n "waitSplitKeyspace\|saveKeyspaceGroups\|labeler\|RegionLabel\|createKeyspaceWithoutCheck\|txnOp\|txnCb" pkg/keyspace/keyspace.go | head -80
Length of output: 1357
🏁 Script executed:
#!/bin/bash
# View the full createKeyspaceWithoutCheck function
sed -n '290,375p' pkg/keyspace/keyspace.go
Length of output: 2402
🏁 Script executed:
#!/bin/bash
# View saveKeyspaceRegionLabelerTxnOp implementation
sed -n '425,480p' pkg/keyspace/keyspace.go
Length of output: 1811
@bufferflies, thanks for the clarification — I dug into the code to understand your point, but the evidence points the other way.
Looking at createKeyspaceWithoutCheck (lines 292–375 of keyspace.go):
-
saveKeyspaceRegionLabelerTxnOpis called at line 322, before the transaction runs — it is not guarded bywaitSplit. However, it uses theskipSplitRegionfailpoint (line 426), notwaitSplitKeyspaceFailed. And sinceSetupSuitealready enablesskipSplitRegionsuite-wide, this function returnsnil, nil, nilfor every test in the suite — meaning the region labeler is never actually set up in these tests. -
waitSplitKeyspaceFailedis injected at line 354, strictly insideif tracer.waitSplit {(line 352). Withmanagerbuilt from&mockConfig{}(whereToWaitRegionSplit()returnsfalse),tracer.waitSplitisfalse, so this block — and the failpoint inside it — is never reached.
These are two separate code paths using separate failpoints. waitSplitKeyspaceFailed does not influence saveKeyspaceRegionLabelerTxnOp.
The test still provides value (verifying that requests[0] can be retried successfully after the saveKeyspaceGroupsTxnOpFailed-induced failure), but the waitSplitKeyspaceFailed enable at line 150 is effectively a no-op in this configuration. Would you consider either:
- Setting
WaitRegionSplit: trueon the localmanagerconfig in this test (sotracer.waitSplit = trueand the failpoint fires), or - Adding a note in the test comment that this section only validates retry-after-failure, not the post-commit split error path?
🧠 Learnings used
Learnt from: bufferflies
Repo: tikv/pd PR: 10492
File: pkg/keyspace/keyspace.go:292-301
Timestamp: 2026-03-27T03:13:21.715Z
Learning: In tikv/pd `pkg/keyspace/keyspace.go`, `createKeyspaceWithoutCheck` intentionally persists the new keyspace as `keyspacepb.KeyspaceState_ENABLED` immediately upon creation (not DISABLED as before). Keyspace state is intentionally decoupled from the region split process. `CheckKeyspaceState` only transiently flips the state on the caller's in-memory copy when `CheckKeyspaceRegionBound` returns false — it does NOT persist the change. This is by design: split readiness is a separate, read-time concern and must not be flagged as inconsistent state handling. Do not raise issues about ENABLED vs DISABLED state being different across code paths due to this pattern.
Learnt from: bufferflies
Repo: tikv/pd PR: 10330
File: pkg/keyspace/util.go:462-485
Timestamp: 2026-03-18T13:01:12.324Z
Learning: In tikv/pd, `GetKeyspaceSplitKeys` in `pkg/keyspace/util.go` is intentionally designed to return split keys for only one keyspace at a time (a single `checker.GetKeyspaceIDInRange` call). The SplitChecker is invoked repeatedly across scheduling cycles to incrementally resolve all cross-keyspace spans, so splitting all boundaries in a single call is not the goal.
Learnt from: CR
Repo: tikv/pd PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-09T06:38:09.379Z
Learning: Applies to **/*_test.go : Tests must handle failpoints using github.com/pingcap/failpoint. Do not run `go test` directly unless failpoints are enabled/disabled correctly. Use `make test`, `make basic-test`, or `make gotest` commands to auto enable/disable failpoints.
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
📒 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>
bufferflies
force-pushed
the
bugfix/create_keyspace_atomic
branch
from
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
📒 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>
bufferflies
force-pushed
the
bugfix/create_keyspace_atomic
branch
from
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.
| 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
|
@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. |
| 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.
| var k api.KeyspaceMeta | ||
| keyspaceName := "keyspace_1" | ||
| testutil.Eventually(re, func() bool { | ||
| fmt.Printf("get keyspace aaaa: %v\n", k) |
3 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