mockstore: implement LoadKeyspaceByID in PD mocks#2016
Conversation
Signed-off-by: JmPotato <github@ipotato.me>
ti-chi-bot
Bot
added
the
dco-signoff: yes
|
[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/S
📝 WalkthroughWalkthroughThis PR updates mock and test PD client stubs to align with a new meta-storage API. Both ChangesPD Client Stub Updates
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested labels: component/client, type/enhancement Suggested reviewers: haojinming 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
@JmPotato: The following test 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/mockstore/mocktikv/pd.go`:
- Line 494: Update the Watch API return type to match the PD client’s
metastorage contract: the pdClient.Watch method should return a channel of
[]*meta_storagepb.Event instead of chan *metastorage.WatchResponse, and the
corresponding test helper in txnkv/transaction/test_util.go should use the same
element type. Make sure any WatchResponse-specific handling is removed or
adapted so the mock aligns with the clients/metastorage Watch signature.
In `@txnkv/transaction/test_util.go`:
- Line 344: The unimplemented PD client Watch signature is using an outdated
return type, which breaks compilation against the pinned pd client version.
Update the Watch method on unimplementedPDClient to match pd.Client.Watch by
returning chan []*meta_storagepb.Event instead of chan
*metastorage.WatchResponse, and ensure the surrounding transaction test
utilities use the same event type consistently.
🪄 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: af356144-6987-4f6f-a9e5-a389122081b3
📒 Files selected for processing (2)
internal/mockstore/mocktikv/pd.gotxnkv/transaction/test_util.go
| } | ||
|
|
||
| func (c *pdClient) Watch(ctx context.Context, key []byte, opts ...opt.MetaStorageOption) (chan []*meta_storagepb.Event, error) { | ||
| func (c *pdClient) Watch(ctx context.Context, key []byte, opts ...opt.MetaStorageOption) (chan *metastorage.WatchResponse, error) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the vendored/pinned pd client's metastorage package for the actual exported types.
fd -t d 'metastorage' | xargs -I{} find {} -name '*.go' | xargs -I{} sh -c 'echo "== {} =="; grep -n "^type\|^func" {}'Repository: tikv/client-go
Length of output: 152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '== go.mod ==\n'
sed -n '1,220p' go.mod
printf '\n== references to WatchResponse and metastorage ==\n'
rg -n "WatchResponse|metastorage\.Watch|metastorage" internal/mockstore/mocktikv/pd.go txnkv/transaction/test_util.go . -g '!**/vendor/**'
printf '\n== pd.go around Watch ==\n'
sed -n '450,560p' internal/mockstore/mocktikv/pd.go
printf '\n== test_util.go relevant section ==\n'
sed -n '1,220p' txnkv/transaction/test_util.goRepository: tikv/client-go
Length of output: 15244
🌐 Web query:
github.com/tikv/pd/client metastorage WatchResponse exported type watch response
💡 Result:
In the TiKV and PD (Placement Driver) ecosystem, WatchResponse is an exported type originating from the kvproto repository [1][2], specifically defined within the meta_storagepb package. While the PD Go client (github.com/tikv/pd/client) interacts with PD, WatchResponse is primarily associated with the gRPC-based Meta Storage API used for watching changes to meta data [1][2]. In the TiKV Rust client implementation, WatchResponse is used as part of the streaming response when a watch request is initiated against the Meta Storage service [1][2]. Key points regarding the usage of WatchResponse: - Definition: It is a protobuf-generated message type defined in kvproto under meta_storagepb [1][2]. - Usage: It is utilized in streaming RPC calls (e.g., in Rust, it is returned via grpcio::ClientSStreamReceiver<WatchResponse>) to handle updates or events related to watched keys or paths [1][2]. - Go Client Context: While the Go client library provides methods like WatchGlobalConfig [3], it manages its own internal channels and response types (e.g., GlobalConfigItem) rather than directly exposing the meta_storagepb.WatchResponse type to the high-level application developer interface [3]. If you are working directly with the generated gRPC client in Go, you would import it from the generated meta_storagepb package [1][2]. If you are encountering issues with WatchResponse (such as memory management or channel handling), developers often look at the underlying gRPC stream processing, as errors in the stream are typically accessible via the Err() method on the response object or during the stream consumption process [4].
Citations:
- 1: https://github.com/tikv/tikv/blob/master/components/pd_client/src/client.rs
- 2: https://github.com/tikv/tikv/blob/c7429059/components/pd_client/src/client.rs
- 3: https://github.com/tikv/pd/blob/master/client/client.go
- 4: Memory leak caused by watch pd#7807
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== go.mod snippets =='
rg -n 'github.com/tikv/pd/client|replace .*pd/client|metastorage' go.mod go.sum
echo
echo '== local module cache candidates =='
go env GOPATH GOMODCACHE
echo
echo '== search module cache for metastorage package files =='
modcache="$(go env GOMODCACHE)"
find "$modcache" -path '*github.com/tikv/pd/client*' -path '*metastorage*' -type f -name '*.go' 2>/dev/null | head -n 50
echo
echo '== search for WatchResponse symbols in cached pd client =='
grep -RIn --include='*.go' 'type .*WatchResponse\|WatchResponse' "$modcache"/github.com/tikv/pd/client* 2>/dev/null | head -n 80Repository: tikv/client-go
Length of output: 808
🏁 Script executed:
#!/bin/bash
set -euo pipefail
modcache="$(go env GOMODCACHE)"
file="$modcache/github.com/tikv/pd/client@v0.0.0-20260601035915-3ef6a3b10c84/clients/metastorage/client.go"
printf '== %s ==\n' "$file"
cat -n "$file" | sed -n '1,260p'
printf '\n== watch-related declarations ==\n'
rg -n 'Watch|WatchResponse|type .*Response|meta_storagepb' "$file"Repository: tikv/client-go
Length of output: 4496
Replace metastorage.WatchResponse with []*meta_storagepb.Event
github.com/tikv/pd/client@v0.0.0-20260601035915-3ef6a3b10c84 exposes Watch(ctx, ...) (chan []*meta_storagepb.Event, error) in clients/metastorage, so both this mock and txnkv/transaction/test_util.go should use that element type.
🧰 Tools
🪛 GitHub Check: golangci
[failure] 494-494:
undefined: metastorage.WatchResponse (typecheck)
🪛 GitHub Check: test
[failure] 494-494:
undefined: metastorage.WatchResponse
🤖 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/mockstore/mocktikv/pd.go` at line 494, Update the Watch API return
type to match the PD client’s metastorage contract: the pdClient.Watch method
should return a channel of []*meta_storagepb.Event instead of chan
*metastorage.WatchResponse, and the corresponding test helper in
txnkv/transaction/test_util.go should use the same element type. Make sure any
WatchResponse-specific handling is removed or adapted so the mock aligns with
the clients/metastorage Watch signature.
Sources: Linters/SAST tools, Pipeline failures
|
|
||
| // Watch implements pd.Client. | ||
| func (u *unimplementedPDClient) Watch(ctx context.Context, key []byte, opts ...opt.MetaStorageOption) (chan []*meta_storagepb.Event, error) { | ||
| func (u *unimplementedPDClient) Watch(ctx context.Context, key []byte, opts ...opt.MetaStorageOption) (chan *metastorage.WatchResponse, error) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and nearby references
git ls-files | rg '^txnkv/transaction/test_util\.go$|^internal/mockstore/mocktikv/pd\.go$|metastorage\.WatchResponse|WatchResponse|MetaStorageOption'
# Show the relevant section of txnkv/transaction/test_util.go
if [ -f txnkv/transaction/test_util.go ]; then
sed -n '300,380p' txnkv/transaction/test_util.go
fi
# Show the relevant section of internal/mockstore/mocktikv/pd.go if present
if [ -f internal/mockstore/mocktikv/pd.go ]; then
sed -n '1,220p' internal/mockstore/mocktikv/pd.go
fi
# Search for the Watch method and WatchResponse type usages across the repo
rg -n "func \(u \*unimplementedPDClient\) Watch|metastorage\.WatchResponse|type WatchResponse|WatchResponse" .Repository: tikv/client-go
Length of output: 11025
🏁 Script executed:
set -euo pipefail
# Show the pd/client version pinned by this repo
rg -n 'github.com/tikv/pd/client|github.com/tikv/pd' go.mod go.sum
# Search for any local references to metastorage.WatchResponse or WatchResponse
rg -n 'metastorage\.WatchResponse|type WatchResponse|WatchResponse' .
# Inspect the pd.Client-facing watch methods around the mock implementation
sed -n '470,520p' internal/mockstore/mocktikv/pd.go
sed -n '330,360p' txnkv/transaction/test_util.goRepository: tikv/client-go
Length of output: 3925
🏁 Script executed:
set -euo pipefail
echo "GOMODCACHE=$(go env GOMODCACHE)"
echo "GOPATH=$(go env GOPATH)"
modcache="$(go env GOMODCACHE)"
pdroot="$modcache/github.com/tikv/pd/client@v0.0.0-20260601035915-3ef6a3b10c84"
if [ -d "$pdroot" ]; then
echo "FOUND_MODULE_CACHE=$pdroot"
rg -n "type WatchResponse|WatchResponse" "$pdroot"/clients/metastorage "$pdroot"/clients -g'*.go'
sed -n '1,220p' "$pdroot"/clients/metastorage/*.go 2>/dev/null || true
else
echo "Module cache directory not found: $pdroot"
fiRepository: tikv/client-go
Length of output: 320
🏁 Script executed:
set -euo pipefail
modcache="$(go env GOMODCACHE)"
pdroot="$modcache/github.com/tikv/pd/client@v0.0.0-20260601035915-3ef6a3b10c84"
echo "Files under clients/metastorage:"
find "$pdroot/clients/metastorage" -maxdepth 2 -type f | sort
echo
echo "Search for WatchResponse and package declarations:"
rg -n "type WatchResponse|WatchResponse|package " "$pdroot/clients/metastorage" -g'*.go'
echo
echo "Show the metastorage files:"
for f in "$pdroot"/clients/metastorage/*.go; do
echo "---- $f ----"
sed -n '1,220p' "$f"
doneRepository: tikv/client-go
Length of output: 3805
Use chan []*meta_storagepb.Event here
metastorage.WatchResponse doesn’t exist in the pinned github.com/tikv/pd/client version; pd.Client.Watch now returns chan []*meta_storagepb.Event, so this signature breaks the build.
🧰 Tools
🪛 GitHub Check: test
[failure] 344-344:
undefined: metastorage.WatchResponse
🤖 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 `@txnkv/transaction/test_util.go` at line 344, The unimplemented PD client
Watch signature is using an outdated return type, which breaks compilation
against the pinned pd client version. Update the Watch method on
unimplementedPDClient to match pd.Client.Watch by returning chan
[]*meta_storagepb.Event instead of chan *metastorage.WatchResponse, and ensure
the surrounding transaction test utilities use the same event type consistently.
Sources: Linters/SAST tools, Pipeline failures
1 participant
Summary by CodeRabbit