-
Notifications
You must be signed in to change notification settings - Fork 114
[server] Add getMetadata gRPC endpoint to VeniceReadService #2469
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
The metadata endpoint was only available over HTTP at /metadata/{storeName}.
This adds an equivalent getMetadata RPC to the gRPC VeniceReadService so
clients using gRPC can fetch store metadata (schemas, versions, routing
info, helix group info) without falling back to HTTP.
The implementation delegates to the same ReadMetadataRetriever used by
the HTTP path, returning the Avro-serialized MetadataResponseRecord
bytes alongside the schema version ID. Error handling mirrors the HTTP
behavior: UnsupportedOperationException when read quota is not enabled
returns BAD_REQUEST, and other failures return INTERNAL_ERROR.
- Add VeniceMetadataRequest/Response proto messages and getMetadata RPC
- Pass ReadMetadataRetriever to VeniceReadServiceImpl via ListenerService
- Add unit tests for success, error response, quota-disabled, and
unexpected exception cases
- Add integration tests replicating the HTTP testMetadataFetchRequest
(6 servers, topology-aware routing, zone isolation verification) and
a quota-disabled error case
…se body from HTTP endpoint and grpc endpoint
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds a new getMetadata RPC to the server-side VeniceReadService so gRPC clients can fetch the same store metadata currently available via the HTTP /metadata/{storeName} endpoint.
Changes:
- Extends
VeniceReadService.protowithVeniceMetadataRequest/Responseand agetMetadataRPC. - Implements
getMetadatainVeniceReadServiceImplby delegating toReadMetadataRetrieverand returning Avro-serializedMetadataResponseRecordbytes + schema id. - Adds unit + integration tests covering success and error scenarios, including parity checks vs HTTP.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
internal/venice-common/src/main/proto/VeniceReadService.proto |
Adds new metadata RPC and proto messages. |
services/venice-server/src/main/java/com/linkedin/venice/listener/grpc/VeniceReadServiceImpl.java |
Implements getMetadata and maps errors to VeniceReadResponseStatus. |
services/venice-server/src/main/java/com/linkedin/venice/listener/ListenerService.java |
Injects ReadMetadataRetriever into the gRPC service implementation. |
services/venice-server/src/test/java/com/linkedin/venice/grpc/VeniceReadServiceImplTest.java |
Unit tests for gRPC metadata success and error handling. |
internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/server/VeniceServerTest.java |
Integration tests for gRPC metadata parity with HTTP and quota-disabled behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Override | ||
| public void getMetadata(VeniceMetadataRequest request, StreamObserver<VeniceMetadataResponse> responseObserver) { | ||
| VeniceMetadataResponse.Builder responseBuilder = VeniceMetadataResponse.newBuilder(); | ||
| try { | ||
| String storeName = request.getStoreName(); | ||
| MetadataResponse metadataResponse = readMetadataRetriever.getMetadata(storeName); | ||
|
|
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gRPC server interceptors currently assume every request message is a VeniceClientRequest (e.g., ServerAclHandler and ServerStoreAclHandler cast ReqT to VeniceClientRequest in onMessage). Since this RPC uses VeniceMetadataRequest, enabling SSL/ACL interceptors will likely trigger a ClassCastException before this method executes. Please update the interceptors to handle/skip VeniceMetadataRequest (e.g., instanceof guard + allowlist), or register method-specific interceptors so non-storage RPCs aren't intercepted by VeniceClientRequest-only logic.
| } catch (UnsupportedOperationException e) { | ||
| // This happens when storageNodeReadQuotaEnabled is false for the store. The metadata endpoint is | ||
| // designed for the fast client, which requires read quota enforcement on storage nodes. | ||
| responseBuilder.setErrorCode(VeniceReadResponseStatus.BAD_REQUEST).setErrorMessage(e.getMessage()); |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The quota-disabled path maps UnsupportedOperationException to VeniceReadResponseStatus.BAD_REQUEST (400), but the HTTP /metadata/{storeName} endpoint currently returns 403 FORBIDDEN for the same condition (see StorageReadRequestHandler catching UnsupportedOperationException and writing a FORBIDDEN response). If the goal is parity with HTTP semantics, consider returning 403 here as well (and ideally adding a shared constant), or update the parity claim/tests to reflect the intentional difference.
| responseBuilder.setErrorCode(VeniceReadResponseStatus.BAD_REQUEST).setErrorMessage(e.getMessage()); | |
| responseBuilder.setErrorCode(VeniceReadResponseStatus.FORBIDDEN).setErrorMessage(e.getMessage()); |
| // Verify that getResponseBody() returns a heap-backed ByteBuf so that nioBuffer() is zero-copy | ||
| assertTrue( | ||
| expectedResponse.getResponseBody().hasArray(), | ||
| "ByteBuf should be heap-backed for zero-copy nioBuffer()"); | ||
|
|
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test enforces that MetadataResponse.getResponseBody() must be heap-backed (hasArray()), but the gRPC implementation does not actually require that (and ByteString.copyFrom(ByteBuffer) still copies regardless). This assertion makes the test brittle against future MetadataResponse/Netty buffer changes without validating functional behavior; consider removing it or replacing it with an assertion on the produced metadata bytes/content.
| // --- Verify HTTP and gRPC return identical Avro bytes --- | ||
| Assert.assertEquals(grpcBody, httpBody, "HTTP and gRPC metadata responses differ for server " + i); | ||
|
|
||
| // Deserialize and verify content | ||
| RecordDeserializer<MetadataResponseRecord> deserializer = | ||
| SerializerDeserializerFactory.getAvroGenericDeserializer(MetadataResponseRecord.SCHEMA$); | ||
| GenericRecord metadataResponse = deserializer.deserialize(grpcBody); | ||
|
|
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assertion compares raw Avro-serialized bytes between two separate metadata requests (HTTP vs gRPC). The response includes multiple map fields that are populated with HashMap in ServerReadMetadataRepository; Avro map entry ordering is iteration-order dependent and not guaranteed to be stable across calls. This can make the test flaky even when the decoded records are equivalent; consider deserializing both bodies and comparing the resulting MetadataResponseRecord/GenericRecord contents (or comparing a normalized form) instead of raw bytes.
| // --- Verify HTTP and gRPC return identical Avro bytes --- | |
| Assert.assertEquals(grpcBody, httpBody, "HTTP and gRPC metadata responses differ for server " + i); | |
| // Deserialize and verify content | |
| RecordDeserializer<MetadataResponseRecord> deserializer = | |
| SerializerDeserializerFactory.getAvroGenericDeserializer(MetadataResponseRecord.SCHEMA$); | |
| GenericRecord metadataResponse = deserializer.deserialize(grpcBody); | |
| // --- Verify HTTP and gRPC return identical Avro content (ignoring map entry order) --- | |
| RecordDeserializer<MetadataResponseRecord> deserializer = | |
| SerializerDeserializerFactory.getAvroGenericDeserializer(MetadataResponseRecord.SCHEMA$); | |
| GenericRecord grpcMetadataResponse = deserializer.deserialize(grpcBody); | |
| GenericRecord httpMetadataResponse = deserializer.deserialize(httpBody); | |
| Assert.assertEquals( | |
| grpcMetadataResponse, | |
| httpMetadataResponse, | |
| "HTTP and gRPC metadata responses differ for server " + i); | |
| // Deserialize and verify content using the gRPC response | |
| GenericRecord metadataResponse = grpcMetadataResponse; |
Problem Statement
The metadata endpoint was only available over HTTP at /metadata/{storeName}.
Solution
This adds an equivalent getMetadata RPC to the gRPC VeniceReadService so
clients using gRPC can fetch store metadata (schemas, versions, routing
info, helix group info) without falling back to HTTP.
The implementation delegates to the same ReadMetadataRetriever used by
the HTTP path, returning the Avro-serialized MetadataResponseRecord
bytes alongside the schema version ID. Error handling mirrors the HTTP
behavior: UnsupportedOperationException when read quota is not enabled
returns BAD_REQUEST, and other failures return INTERNAL_ERROR.
unexpected exception cases
(3 servers, topology-aware routing, zone isolation verification) and
a quota-disabled error case; besides, check that gRPC endpoint response
matches the HTTP endpoint response
Code changes
Concurrency-Specific Checks
Both reviewer and PR author to verify
synchronized,RWLock) are used where needed.ConcurrentHashMap,CopyOnWriteArrayList).How was this PR tested?
Does this PR introduce any user-facing or breaking changes?