*: update kvproto to apiv3 - #2024
Conversation
Signed-off-by: disksing <i@disksing.com>
ti-chi-bot
Bot
added
dco-signoff: yes
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates keyspace protobuf handling to use wrapper fields and generated accessors. Refreshes ChangesKeyspace compatibility and module updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/apicodec/codec_v2_test.go (1)
887-887: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSwap arguments to match
testify's expected vs. actual convention.In
testify/suite, theEqualmethod expects arguments in the order(expected, actual). Swapping them ensures that any failure messages will report the expected and actual values correctly instead of inversely.♻️ Proposed refactor
- suite.Equal(task.Meta.GetKeyspaceId(), testKeyspaceID) + suite.Equal(testKeyspaceID, task.Meta.GetKeyspaceId())🤖 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 `@internal/apicodec/codec_v2_test.go` at line 887, Update the Equal assertion in the relevant test to pass testKeyspaceID as the expected value and task.Meta.GetKeyspaceId() as the actual value, preserving the existing comparison.
🤖 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 `@internal/apicodec/codec_v2_test.go`:
- Line 887: Update the Equal assertion in the relevant test to pass
testKeyspaceID as the expected value and task.Meta.GetKeyspaceId() as the actual
value, preserving the existing comparison.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 028004a3-d722-4842-84e1-09af2e9ec1ee
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumintegration_tests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
examples/gcworker/go.modexamples/rawkv/go.modexamples/txnkv/1pc_txn/go.modexamples/txnkv/async_commit/go.modexamples/txnkv/delete_range/go.modexamples/txnkv/go.modexamples/txnkv/pessimistic_txn/go.modexamples/txnkv/unsafedestoryrange/go.modgo.modintegration_tests/go.modinternal/apicodec/codec.gointernal/apicodec/codec_v2.gointernal/apicodec/codec_v2_test.gointernal/locate/pd_codec.gotikv/compatible_txn_safe_point_loader.gotikv/test_util.gotikvrpc/tikvrpc_test.go
Signed-off-by: disksing <i@disksing.com>
|
Addressed the CodeRabbit nit in 901d142 by using the expected/actual argument order. This update also moves integration_tests to the APIV3-compatible TiDB revision and fixes the remaining KeyspaceMeta getter; the full integration test module now compiles. |
Signed-off-by: disksing <i@disksing.com>
Signed-off-by: disksing <i@disksing.com>
# Conflicts: # examples/gcworker/go.mod # examples/rawkv/go.mod # examples/txnkv/1pc_txn/go.mod # examples/txnkv/async_commit/go.mod # examples/txnkv/delete_range/go.mod # examples/txnkv/go.mod # examples/txnkv/pessimistic_txn/go.mod # examples/txnkv/unsafedestoryrange/go.mod # go.mod # go.sum # integration_tests/go.mod # integration_tests/go.sum
Signed-off-by: disksing <i@disksing.com>
ti-chi-bot
Bot
added
size/M
Signed-off-by: disksing <i@disksing.com>
- Update github.com/tikv/pd/client to the latest master revision (ba23e721a) in the root, integration-test, and example modules. - Remove the fragile assertion that the background goroutine has not finished right after Commit returns, in Test2PCLifecycleHooks, Test2PCCleanupLifecycleHooks and TestAsyncCommitLifecycleHooks. With the mock store's fast commit path the Post hook may run before Commit returns, so the assertion is racy and fails intermittently under -race on CI. Signed-off-by: disksing <i@disksing.com>
ti-chi-bot
Bot
added
size/XL
|
CI follow-up:
|
|
/test pull-integration-test-nextgen |
…ation Since kvproto moved Context.keyspace into a oneof, setAPICtx built a new *Context_KeyspaceId wrapper for every encoded V1/V2 request, which escapes to the heap and adds 4 B/1 alloc on the default RPC hot path (0 B/0 allocs before). The keyspace ID of a codec never changes, so cache the wrapper in the codec and reuse it for all encoded requests. setAPICtx now reports 0 B/0 allocs again. Signed-off-by: disksing <i@disksing.com>
|
@cfzjywxk Done - the PR description now references the design issue: pingcap/kvproto#1506 (introduce namespace-scoped keyspace identity for API V3). The protos themselves were added in pingcap/kvproto#1458. |
KeyspaceMeta.GetId() returns 0 when the keyspace oneof holds a KeyspaceIdentity (API V3), and a zero numeric ID is invalid under API V3 semantics. NewCodecV2 is exported as tikv.NewCodecV2, so passing a V3 meta previously succeeded silently as a default-keyspace codec and encoded keys and request context for keyspace 0. Reject the identity arm explicitly since codec V2 only supports the numeric keyspace ID. Signed-off-by: disksing <i@disksing.com>
ekexium
left a comment
ekexium
left a comment
There was a problem hiding this comment.
LGTM after fixing existing comments
NewCodecV2 used to read keyspaceMeta.Id directly, so a nil meta failed immediately; the nil-safe generated getters now silently produced a default-keyspace (ID 0) codec. Restore the loud failure for nil meta. GetKeyspaceID reports the numeric keyspace ID from PD metadata. When the keyspace oneof holds an API V3 KeyspaceIdentity, GetId() returns 0, so the helper silently routed an identity-scoped keyspace as keyspace 0. Reject the identity arm explicitly, matching NewCodecV2. Add tests for both cases. Signed-off-by: disksing <i@disksing.com>
ti-chi-bot
Bot
added
size/XXL
Update github.com/tikv/pd/client to the latest master revision afa43111d (client: recognize v2 keyspace-level GC, #11100) in the root, integration-test, and example modules. Signed-off-by: disksing <i@disksing.com>
de95a8c to
0989abf
Compare
|
Updated |
|
@ekexium All of rleungx's review comments have been addressed and replied to in the threads:
CI is green (all GitHub Actions jobs + prow nextgen). Could you take another look / approve? |
…-proto-fix Signed-off-by: disksing <i@disksing.com>
ti-chi-bot
Bot
added
lgtm
approved
and removed
needs-1-more-lgtm
|
/hold |
ti-chi-bot
Bot
added
the
do-not-merge/hold
|
@pingyu Thanks for the approval. The PR now has the required approvals and |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfzjywxk, pingyu, rleungx The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
ti-chi-bot
Bot
removed
the
do-not-merge/hold
5 participants
What problem does this PR solve?
The kvproto APIV3 branch moves keyspace-related protobuf fields into oneofs, so the current client code no longer compiles against the latest definitions.
Issue: pingcap/kvproto#1506 (introduce namespace-scoped keyspace identity for API V3); the protos are added by pingcap/kvproto#1458.
What changed?
ba23e721a(2026-08-04) in the root, integration-test, and example modules.*Context_KeyspaceIdinstead of allocating a new wrapper on every encoded V1/V2 request. The isolatedsetAPICtxpath goes back to 0 B/0 allocs per call (was 4 B/1 alloc).Test2PCLifecycleHooks,Test2PCCleanupLifecycleHooksandTestAsyncCommitLifecycleHooksassertedreachedPost == falseimmediately afterCommitreturned, assuming the background (secondary commit / cleanup) goroutine had not finished yet. With the mock store that goroutine can finish beforeCommitreturns, so the assertion is racy andintegration-local-racefailed intermittently. The fragile assertion is removed; the tests still verify thatPreruns and thatPosteventually runs.Tests
go test -tags=intest ./...go test -race ./...inintegration_tests/Summary by CodeRabbit