[protocol] Add new gRPC RPC definitions and messages for read service #2464
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add new RPCs to VeniceReadService proto: singleGet, multiGet, multiGetStreaming, compute, computeStreaming, isServerHealthy, getCompressionDictionary, handleAdminRequest, getMetadata, getCurrentVersionInfo, getIngestionContext. Add corresponding request/response messages with proper field types. Existing get/batchGet/countByValue RPCs remain unchanged for backward compatibility. No behavioral changes - proto-only additions.
There was a problem hiding this comment.
Pull request overview
Adds the next set of gRPC read-service RPCs and message definitions to VeniceReadService.proto as part of the read service rewrite, while keeping existing RPCs for backward compatibility.
Changes:
- Added 11 new RPC methods to
VeniceReadService(including streaming variants). - Introduced request/response messages for single-get, multi-get, compute, health, metadata, admin, and ingestion context operations.
- Added shared helper messages for multi-key requests and RPC headers.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Change MultiKeyRequestKey.keyIndex from uint32 to int32 for consistency with MultiKeyStreamingResponse.keyIndex (sint32) and Java's signed int - Add comments explaining keyCount purpose (buffer pre-allocation) - Make all errorMessage fields consistently optional across responses
- Define proto enum matching Java ServerAdminAction with UNSPECIFIED = 0 - Values offset by 1 from Java enum to accommodate proto3 default - Update AdminRequest to use enum instead of string
sushantmane
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Following the pattern from controller gRPC (PR linkedin#1454), errors should be sent via gRPC's native error handling (responseObserver.onError with StatusRuntimeException and google.rpc.Status) rather than embedded in response messages. Removed errorMessage from: SingleGetResponse, MultiKeyResponse, MultiKeyStreamingResponse, CompressionDictionaryResponse, AdminResponse, CurrentVersionInfoResponse, MetadataResponse, IngestionContextResponse. Kept errorMessage in existing VeniceServerResponse and CountByValueResponse for backward compatibility with implemented RPCs.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… add interceptor TODO - Rename `rcu` to `responseRCU` in SingleGetResponse, MultiKeyResponse, and MultiKeyStreamingResponse for consistency with VeniceServerResponse - Fix ServerAdminAction enum comment to accurately document the +1 offset mapping between proto values and Java ServerAdminAction.getValue() - Add TODO noting gRPC interceptors must be updated to handle new request types before enabling the new RPCs
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
VeniceReadServiceproto:singleGet,multiGet,multiGetStreaming,compute,computeStreaming,isServerHealthy,getCompressionDictionary,handleAdminRequest,getMetadata,getCurrentVersionInfo,getIngestionContextSingleGetRequest/Response,MultiGetRequest,MultiKeyResponse,MultiKeyStreamingResponse,ComputeRequest,HealthCheckRequest/Response,CompressionDictionaryRequest/Response,AdminRequest/Response,MetadataRequest/Response,CurrentVersionInfoRequest/Response,IngestionContextRequest/ResponseMultiKeyRequestKey,RpcRequestHeaderget/batchGet/countByValueRPCs remain unchanged for backward compatibilityThis is PR 2 of the gRPC read service rewrite series. Proto-only changes — no behavioral changes.
Test plan
./gradlew :internal:venice-common:build— proto compiles, all tests pass./gradlew :services:venice-server:build— server builds with new generated stubs./gradlew :clients:venice-client:build— client builds, no regressions