[feat(router service)]:support new option for WithAllowRouterServiceHandle for access router service#1797
Conversation
e304dc4 to
89b38ef
Compare
aa1520e to
6652825
Compare
📝 WalkthroughWalkthroughAdded router-service PD options to region and store lookups; extended store fetch API to accept variadic GetStore options (one-time router option on initial resolve); updated PD interceptor, mocks, and tests to forward GetStore options; bumped PD client and multiple indirect Go module versions across examples and integration tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Cache as Cache (region/store)
participant PD as PD Client
participant Router as Router Service
participant Store as Store Meta
Cache->>PD: GetRegion/GetStore(ctx, id, opts: AllowFollowerHandle, AllowRouterServiceHandle)
PD->>Router: route request (router-service option present)
Router-->>PD: routed response
PD->>Store: GetStore(ctx, storeID, opts...)
Store-->>PD: store metadata
PD-->>Cache: return region/store metadata
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Signed-off-by: bufferflies <1045931706@qq.com>
6652825 to
a88bf8c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/locate/store_cache.go`:
- Around line 59-61: Update the method signature of storeCacheImpl.fetchStore to
accept the new variadic options parameter so it matches storeRegistry: change
fetchStore(ctx context.Context, id uint64) to fetchStore(ctx context.Context, id
uint64, opts ...opt.GetStoreOption); within the method forward those opts when
calling the underlying registry/fetch (and update any internal calls to pass
through opts), and update any places that call storeCacheImpl.fetchStore to
supply or forward the options as needed so the implementation satisfies the
storeRegistry interface.
♻️ Duplicate comments (5)
examples/txnkv/async_commit/go.mod (1)
34-34: Same pd/client bump verification applies here.
Please see the verification request in the root go.mod review comment.examples/gcworker/go.mod (1)
34-34: Same pd/client bump verification applies here.
Please see the verification request in the root go.mod review comment.examples/txnkv/pessimistic_txn/go.mod (1)
34-34: Same pd/client bump verification applies here.
Please see the verification request in the root go.mod review comment.examples/rawkv/go.mod (1)
34-34: Same pd/client bump verification applies here.
Please see the verification request in the root go.mod review comment.examples/txnkv/delete_range/go.mod (1)
34-34: Same pd/client bump verification applies here.
Please see the verification request in the root go.mod review comment.
8384a76 to
499d3d9
Compare
Signed-off-by: bufferflies <1045931706@qq.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@integration_tests/go.mod`:
- Around line 182-183: Update the PR and repository notes to document the fork
usage: state whether the TiDB changes in the go.mod replace directives
(github.com/pingcap/tidb => github.com/bufferflies/tidb and
github.com/pingcap/tidb/pkg/parser => github.com/bufferflies/tidb/pkg/parser)
are intended to be upstreamed to pingcap/tidb, provide a concrete
timeline/milestones for replacing the fork with canonical versions before
merging to master, and enumerate the exact patches/commits or feature diffs from
the fork required to support the router service so reviewers and downstreams can
reproduce and evaluate the changes.
🧹 Nitpick comments (1)
integration_tests/go.mod (1)
37-38: Check for security advisories in updated indirect dependencies.Recent CVEs have been published for
golang.org/x/cryptoandgolang.org/x/net. While the versions in this update (v0.47.0 and v0.49.0 respectively) are patched versions, it's good practice to review the security changelog:
golang.org/x/crypto— Several SSH-related DoS CVEs fixed in v0.45.0 and earliergolang.org/x/net— NO_PROXY and HTML parsing CVEs addressed in recent patches- Other dependencies (
gopkg.in/ini.v1, Alibaba Cloud packages) have no known public advisories at their specified versions
| github.com/pingcap/tidb => github.com/bufferflies/tidb v0.0.0-20260121115107-e127d738bb02 | ||
| github.com/pingcap/tidb/pkg/parser => github.com/bufferflies/tidb/pkg/parser v0.0.0-20260121115107-e127d738bb02 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if integration_tests/go.mod exists and read the relevant lines
if [ -f "integration_tests/go.mod" ]; then
echo "=== File exists. Reading lines 180-185 for context ==="
sed -n '180,185p' integration_tests/go.mod | cat -n
echo ""
echo "=== Total lines in file ==="
wc -l integration_tests/go.mod
else
echo "File not found at integration_tests/go.mod"
# Try to locate it
find . -name "go.mod" -type f 2>/dev/null | head -10
fiRepository: tikv/client-go
Length of output: 561
🏁 Script executed:
#!/bin/bash
# Search for recent commits or messages related to bufferflies or this fork
echo "=== Searching git log for 'bufferflies' or 'tidb' fork references ==="
git log --oneline --all --grep="bufferflies\|fork" 2>/dev/null | head -20
echo ""
echo "=== Searching for any documentation about this fork ==="
find . -type f \( -name "*.md" -o -name "*.txt" \) -exec grep -l "bufferflies\|fork.*tidb" {} \; 2>/dev/null | head -10Repository: tikv/client-go
Length of output: 663
🏁 Script executed:
#!/bin/bash
# Check recent changes to go.mod file
echo "=== Recent commits touching go.mod files ==="
git log --oneline --all -- "**/go.mod" 2>/dev/null | head -15
echo ""
echo "=== Check if there's a go.mod in root or other locations ==="
find . -maxdepth 3 -name "go.mod" -type f 2>/dev/nullRepository: tikv/client-go
Length of output: 1315
Fork replacement requires clarification on upstreaming and timeline.
The replace directives point to a fork (bufferflies/tidb) instead of the canonical pingcap/tidb repository. While this may be acceptable for development/testing, this setup:
- Prevents reproducible builds in downstream consumers
- Requires clarity on what changes in the fork are necessary
- Needs a defined timeline and plan for migrating back to canonical versions before merging to master
Please clarify:
- Are these TiDB changes planned to be upstreamed to pingcap/tidb?
- What is the timeline for replacing this fork with canonical versions?
- What specific changes in the fork are required for the router service support?
🤖 Prompt for AI Agents
In `@integration_tests/go.mod` around lines 182 - 183, Update the PR and
repository notes to document the fork usage: state whether the TiDB changes in
the go.mod replace directives (github.com/pingcap/tidb =>
github.com/bufferflies/tidb and github.com/pingcap/tidb/pkg/parser =>
github.com/bufferflies/tidb/pkg/parser) are intended to be upstreamed to
pingcap/tidb, provide a concrete timeline/milestones for replacing the fork with
canonical versions before merging to master, and enumerate the exact
patches/commits or feature diffs from the fork required to support the router
service so reviewers and downstreams can reproduce and evaluate the changes.
|
@okJiang: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Signed-off-by: bufferflies <1045931706@qq.com>
5a17c62 to
2a64572
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@examples/rawkv/go.mod`:
- Line 34: Confirm and validate the updated Go module dependencies by checking
that github.com/tikv/pd/client v0.0.0-20260122075414-848dd85011e9,
google.golang.org/grpc v1.75.1, and google.golang.org/protobuf v1.36.6 are
intentional and secure: run go mod tidy to resolve and clean modules, run a
vulnerability scan (e.g., go list -json ./... | nancy sleuth or `govulncheck`)
against these modules, and if any advisories appear pin or roll back the
offending module(s) or update to a patched release; ensure the final
go.mod/go.sum reflect the chosen, verified versions for
github.com/tikv/pd/client, google.golang.org/grpc, and
google.golang.org/protobuf.
In `@examples/txnkv/async_commit/go.mod`:
- Around line 34-49: Update the vulnerable dependency golang.org/x/net to
v0.45.0 or later in the module list (replace the existing golang.org/x/net
v0.41.0 entry), and verify the availability of the pseudo-version for
github.com/tikv/pd/client v0.0.0-20260122075414 by querying the Go module proxy
or listing available versions (use go list -m -versions or fetch the proxy
metadata) before committing; adjust the tikv/pd/client version to a resolvable
tag or commit if the pseudo-version is not present.
In `@examples/txnkv/delete_range/go.mod`:
- Around line 34-49: The google.golang.org/genproto pseudo-version listed
(v0.0.0-20250707201910-8d1bb00bc6a7) requires Go toolchain >= 1.22.11 and
changes submodule layout; verify your Go version and either upgrade the project
toolchain or pin genproto to a compatible release (or revert to the prior
genproto version) in go.mod, then run go mod tidy and build/test the code to
confirm imports resolve; also consider updating google.golang.org/grpc and
google.golang.org/protobuf to their newer patch releases (e.g., grpc v1.76.0,
protobuf v1.36.11) if you want the latest patches.
♻️ Duplicate comments (1)
integration_tests/go.mod (1)
183-185: Fork dependency issue previously flagged.The replace directives pointing to
github.com/bufferflies/tidbfork have already been identified in previous review comments. Please address the concerns raised about upstreaming timeline and migration plan.
Signed-off-by: bufferflies <1045931706@qq.com>
[LGTM Timeline notifier]Timeline:
|
|
/ping @you06 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lcwangchao, okJiang, rleungx, you06 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 |
tidb dep ref : https://github.com/pingcap/tidb/compare/master...bufferflies:tidb:feat/support_router?expand=1
Summary by CodeRabbit
Chores
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.