Repository navigation
Conversation
The generated protobuf messages and gRPC clients were private, so an application that needed an RPC this crate does not wrap had to vendor kvproto and generate its own copy of the same types: TiCDC's `ChangeData/EventFeed`, PD's GC safe point RPCs (`GetGCSafePoint`, `UpdateServiceGCSafePoint`), keyspace management, and so on. A few generated types already leak through the public API (`ProtoLockInfo`, `ProtoKeyError`, `ProtoRegionError`), but not the modules they live in. `tikv_client::proto` is now public, and it re-exports the `tonic` and `prost` versions the clients are generated for, so callers can build a `Channel` and use `Message` without pinning matching versions themselves. The module docs say that the generated code follows kvproto rather than the rest of the API, and show a PD GC safe point call. Rustdoc lints are allowed on the generated code, as clippy lints already are. Tests: `tests/proto_tests.rs` (no cluster) encodes a generated message with the re-exported prost and builds PD and ChangeData clients on a lazy channel from the re-exported tonic; the module doc example is compile-tested. Signed-off-by: Dinakaran <dinakaranvijayakumar@outlook.com>
|
[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 |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe crate now exposes the generated protobuf module and re-exports ChangesProtobuf API
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Feature Merge Risk: ⚪ Minimal · up to The generated protobuf API and documented usage appear compatible. No concrete regression is evident; the change appears mergeable subject to normal build and doctest checks. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Applications can now use PD and TiCDC RPC clients directly, but those clients do not inherit the crate’s configured TLS credentials. The new PD example uses an HTTP channel. This is a security-sensitive API boundary, although no deployed use or server-side authorization bypass is established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Some tools did not complete. Review the errors below. 🔧 Clippy (1.98.1)Clippy execution failed 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 |
Motivation
tikv_client::proto(the generated kvproto messages and gRPC clients) is private. An application that needs an RPC this crate does not wrap has to vendor kvproto and generate a second copy of the same types. Examples:ChangeData/EventFeed, for change data capture;GetGCSafePoint,UpdateGCSafePoint,UpdateServiceGCSafePoint), for an application that runs its own GC worker;keyspacepb).We hit this building a service on
tikv-clientthat acts as the cluster's GC worker. We ended up vendoring 14 kvproto files and atonic-buildstep just for four PD RPCs. Some generated types already leak into the public API (ProtoLockInfo,ProtoKeyError,ProtoRegionError), but not the modules they come from.What is changed and how it works
mod protobecomespub mod proto.tikv_client::protore-exportstonicandprost, the versions the clients are generated with. Callers can build aChanneland useprost::Messagewithout pinning matching versions themselves.GetGCSafePointcall. They also say that the generated code follows kvproto, independently of the rest of the API.#[allow(rustdoc::all)]on the generated code, as clippy lints already are. Rustdoc warnings on kvproto comments would otherwise become this crate's.A feature flag was considered and not used: the generated code is compiled anyway, and making it public adds no dependencies or build time. If maintainers would rather gate it (for example behind
proto) or mark it#[doc(hidden)], that is a one-line change.Tests
tests/proto_tests.rs(no cluster needed): encodes and decodes a generatedpdpbmessage with the re-exportedprost, checks thatProtoLockInfois the generatedkvrpcpb::LockInfo, and buildsPdClientandChangeDataClienton a lazy channel from the re-exportedtonic. It does not compile without the change (module proto is private).no_run).cargo test --lib, clippy with-D clippy::all,cargo fmt --check: clean.RUSTDOCFLAGS=-Dwarnings cargo docstill reports only the two warnings it reports on master ([put']intransaction.rs,<Value>inraw/client.rs).Check list
Release note:
tikv_client::protoexposes the generated kvproto messages and gRPC clients, with thetonicandprostthey are built on.Summary by CodeRabbit
prostandtonic, along with documentation and an example for calling a PD RPC.