tikvrpc: add default request origin - #1975
Conversation
ti-chi-bot
Bot
added
do-not-merge/work-in-progress
|
Warning Review limit reached
More reviews will be available in 4 minutes and 30 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughThis PR bumps the github.com/pingcap/kvproto dependency across the main module and example modules, and adds a process-wide default RequestOrigin (with Set/Get APIs and fill logic) applied in NewRequest and AttachContext, plus unit tests validating the behavior. ChangesDependency and RequestOrigin Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
aba2414 to
d3276f1
Compare
Signed-off-by: Ziqian Qin <eke@fastmail.com>
d3276f1 to
94a0a90
Compare
Signed-off-by: Ziqian Qin <eke@fastmail.com>
ti-chi-bot
Bot
added
size/L
Signed-off-by: Ziqian Qin <eke@fastmail.com>
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Pull request overview
Adds a process-wide default request_origin in tikvrpc and ensures it is filled into kvrpcpb.Context when requests are created or when contexts are attached, alongside bumping kvproto to a revision that includes the new enum/field.
Changes:
- Introduce global default request origin setters/getters and apply the default when
Context.request_originis unknown. - Extend
NewRequestandAttachContextto auto-fillrequest_origin. - Bump
github.com/pingcap/kvprotopseudo-version across the root module, integration tests, and examples.
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tikvrpc/tikvrpc.go |
Adds global default request origin and fills it into request contexts during creation/attachment. |
tikvrpc/tikvrpc_test.go |
Adds tests to validate default request origin propagation. |
go.mod |
Bumps github.com/pingcap/kvproto version for the main module. |
go.sum |
Updates checksums for the bumped kvproto version. |
integration_tests/go.mod |
Bumps github.com/pingcap/kvproto for integration test module. |
integration_tests/go.sum |
Updates checksums for the bumped kvproto version in integration tests. |
examples/txnkv/go.mod |
Bumps github.com/pingcap/kvproto for the txnkv examples module. |
examples/txnkv/async_commit/go.mod |
Bumps indirect github.com/pingcap/kvproto for async_commit example. |
examples/txnkv/1pc_txn/go.mod |
Bumps indirect github.com/pingcap/kvproto for 1pc_txn example. |
examples/txnkv/delete_range/go.mod |
Bumps indirect github.com/pingcap/kvproto for delete_range example. |
examples/txnkv/pessimistic_txn/go.mod |
Bumps indirect github.com/pingcap/kvproto for pessimistic_txn example. |
examples/txnkv/unsafedestoryrange/go.mod |
Bumps indirect github.com/pingcap/kvproto for unsafedestoryrange example. |
examples/rawkv/go.mod |
Bumps indirect github.com/pingcap/kvproto for rawkv example. |
examples/gcworker/go.mod |
Bumps indirect github.com/pingcap/kvproto for gcworker example. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tikvrpc/tikvrpc_test.go (1)
115-119: ⚡ Quick winMake the explicit-origin case observable.
This assertion uses the same value as the configured default, so it still passes if explicit origins start getting overwritten by the default. Flip the default before this case so the preservation contract is actually exercised.
Suggested test tweak
+ SetDefaultRequestOrigin(kvrpcpb.RequestOrigin_RequestOriginUnknown) req = NewRequest(CmdGet, &kvrpcpb.GetRequest{}, kvrpcpb.Context{ RequestOrigin: kvrpcpb.RequestOrigin_RequestOriginTiDB, }) require.Equal(t, kvrpcpb.RequestOrigin_RequestOriginTiDB, req.GetRequestOrigin())🤖 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 `@tikvrpc/tikvrpc_test.go` around lines 115 - 119, Test currently verifies an explicit origin using the same value as the configured default, so change the configured default request origin to a different enum value before constructing the explicit-origin request to ensure preservation is exercised; specifically, set the package's configured default request origin to a different kvrpcpb.RequestOrigin (e.g., RequestOrigin_RequestOriginTiKV), call NewRequest(CmdGet, &kvrpcpb.GetRequest{}, kvrpcpb.Context{RequestOrigin: kvrpcpb.RequestOrigin_RequestOriginTiDB}), assert req.GetRequestOrigin() is RequestOrigin_RequestOriginTiDB, and then restore the original default.
🤖 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 `@tikvrpc/tikvrpc.go`:
- Around line 873-875: AttachContext is only mutating the local rpcCtx copy via
fillDefaultRequestOrigin, so the normalized origin isn't persisted onto
req.Context; modify AttachContext to copy the updated rpcCtx back into
req.Context (e.g., req.Context = rpcCtx or equivalent assignment) immediately
after fillDefaultRequestOrigin(&rpcCtx) and before the code that patches the
protobuf request so that subsequent retries see the filled RequestOrigin;
reference function AttachContext, variables req.Context and rpcCtx, and helper
fillDefaultRequestOrigin when making the change.
---
Nitpick comments:
In `@tikvrpc/tikvrpc_test.go`:
- Around line 115-119: Test currently verifies an explicit origin using the same
value as the configured default, so change the configured default request origin
to a different enum value before constructing the explicit-origin request to
ensure preservation is exercised; specifically, set the package's configured
default request origin to a different kvrpcpb.RequestOrigin (e.g.,
RequestOrigin_RequestOriginTiKV), call NewRequest(CmdGet, &kvrpcpb.GetRequest{},
kvrpcpb.Context{RequestOrigin: kvrpcpb.RequestOrigin_RequestOriginTiDB}), assert
req.GetRequestOrigin() is RequestOrigin_RequestOriginTiDB, and then restore the
original default.
🪄 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: ffb0a60a-f50f-4412-9b48-14e542fe01b1
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumintegration_tests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
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.modtikvrpc/tikvrpc.gotikvrpc/tikvrpc_test.go
Signed-off-by: Ziqian Qin <eke@fastmail.com>
Signed-off-by: Ziqian Qin <eke@fastmail.com>
ti-chi-bot
Bot
added
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MyonKeminta, zyguan 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 |
[LGTM Timeline notifier]Timeline:
|
|
/cherry-pick release-nextgen-202603 |
|
/cherry-pick tidb-8.5 |
|
@ekexium: new pull request created to branch DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
@ekexium: new pull request created to branch DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
ref pingcap/tidb#68799 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ref pingcap/tidb#68799 Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io> Signed-off-by: Ziqian Qin <eke@fastmail.com>
ref pingcap/tidb#68799\n\nSigned-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>\nSigned-off-by: Ziqian Qin <eke@fastmail.com>\n\nCo-authored-by: Ziqian Qin <eke@fastmail.com>
What changed
tikvrpc.kvrpcpb.Context.request_originfrom that default when creating or attaching request context.Dependency stack
RequestOriginandContext.request_origin.replaceremains; the root module, integration test module, and example modules are bumped directly to the merged kvproto pseudo-version.Tests
gotestsum --format short-verbose -- ./tikvrpcgo test ./tikvrpc -run '^TestDefaultRequestOrigin$' -racecd integration_tests && go list -mod=readonly ./...for mod in $(find examples -name go.mod -print | sort); do (cd "${mod%/go.mod}" && go list -mod=readonly ./...); donegit diff --checkSummary by CodeRabbit
New Features
Chores
Tests