[common] Convert VeniceReadResponseStatus to enum and add GrpcUtils helpers #2462
Conversation
…elpers Convert VeniceReadResponseStatus from a class with static final int constants to a proper enum with int code field, fromCode() lookup, and new status codes (METHOD_NOT_ALLOWED, REQUEST_TIMEOUT, MISROUTED_STORE_VERSION). Add toByteString utility methods to GrpcUtils for zero-copy byte[]/ByteBuf to protobuf ByteString conversion. Update HttpShortcutResponse to optionally carry VeniceReadResponseStatus. Update all call sites to use .getCode() where proto int is expected.
There was a problem hiding this comment.
Pull request overview
This PR modernizes Venice gRPC read status handling by converting VeniceReadResponseStatus into a real enum with reverse lookup, and introduces GrpcUtils helpers for building protobuf ByteString values to support upcoming gRPC read-service refactors.
Changes:
- Convert
VeniceReadResponseStatusfromstatic final intconstants to anenumwithgetCode()andfromCode(), adding new status codes. - Add
GrpcUtils.toByteString(byte[])andGrpcUtils.toByteString(ByteBuf)helpers plus accompanying tests. - Update server/client handlers and tests to use
status.getCode()where protobufinterror codes are expected, and extendHttpShortcutResponseto optionally carry aVeniceReadResponseStatus.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| services/venice-server/src/test/java/com/linkedin/venice/listener/StorageReadRequestHandlerTest.java | Updates assertions to compare against enum .getCode() values. |
| services/venice-server/src/test/java/com/linkedin/venice/listener/ReadQuotaEnforcementHandlerTest.java | Updates assertions to compare against enum .getCode() values. |
| services/venice-server/src/main/java/com/linkedin/venice/listener/grpc/handlers/GrpcStorageReadRequestHandler.java | Sets gRPC response error codes using .getCode(). |
| services/venice-server/src/main/java/com/linkedin/venice/listener/grpc/handlers/GrpcReadQuotaEnforcementHandler.java | Sets quota enforcement error codes using .getCode(). |
| services/venice-server/src/main/java/com/linkedin/venice/listener/grpc/handlers/GrpcOutboundResponseHandler.java | Sets KEY_NOT_FOUND error code using .getCode(). |
| services/venice-server/src/main/java/com/linkedin/venice/listener/grpc/VeniceReadServiceImpl.java | Initializes response builder with OK.getCode(). |
| internal/venice-common/src/test/java/com/linkedin/venice/response/VeniceReadResponseStatusTest.java | Adds unit tests for enum code mapping and uniqueness. |
| internal/venice-common/src/test/java/com/linkedin/venice/grpc/GrpcUtilsTest.java | Adds unit tests for new toByteString helpers. |
| internal/venice-common/src/main/java/com/linkedin/venice/response/VeniceReadResponseStatus.java | Introduces enum + reverse lookup map for status codes. |
| internal/venice-common/src/main/java/com/linkedin/venice/listener/response/HttpShortcutResponse.java | Adds optional VeniceReadResponseStatus field to shortcut response. |
| internal/venice-common/src/main/java/com/linkedin/venice/grpc/GrpcUtils.java | Adds ByteString helper methods (byte[] and ByteBuf). |
| clients/venice-client/src/main/java/com/linkedin/venice/fastclient/transport/GrpcTransportClient.java | Updates client-side error handling to use .getCode() and fromCode(). |
| clients/da-vinci-client/src/test/java/com/linkedin/davinci/blobtransfer/TestP2PFileTransferServerHandler.java | Updates status-code comparison to use .getCode(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** Wraps a byte array into a {@link ByteString} without copying. */ | ||
| public static ByteString toByteString(byte[] bytes) { | ||
| if (bytes == null || bytes.length == 0) { | ||
| return ByteString.EMPTY; | ||
| } | ||
| return UnsafeByteOperations.unsafeWrap(bytes); | ||
| } |
There was a problem hiding this comment.
toByteString(byte[]) uses UnsafeByteOperations.unsafeWrap(bytes), which aliases the caller-provided array. If the caller reuses/mutates that array (e.g., pooled buffers), the serialized gRPC payload can be corrupted and violate ByteString immutability expectations. Consider either copying (e.g., ByteString.copyFrom) or clearly documenting/enforcing a contract that the input byte[] must never be modified after passing it here (and potentially renaming the method to make the unsafe semantics explicit).
| /** Reads readable bytes from a {@link ByteBuf} and wraps them into a {@link ByteString}. */ | ||
| public static ByteString toByteString(ByteBuf buf) { | ||
| if (buf == null || buf.readableBytes() == 0) { | ||
| return ByteString.EMPTY; | ||
| } | ||
| byte[] bytes = new byte[buf.readableBytes()]; | ||
| buf.getBytes(buf.readerIndex(), bytes); | ||
| return UnsafeByteOperations.unsafeWrap(bytes); |
There was a problem hiding this comment.
PR description mentions adding a “zero-copy” toByteString(ByteBuf) helper, but the current implementation always allocates a new byte[] and copies from the ByteBuf. Either update the PR/docs to reflect that this is a copying conversion, or implement an actual zero-copy path (only when safe) with clearly defined buffer lifetime / reference-counting expectations.
| private static final Map<Integer, VeniceReadResponseStatus> CODE_MAP = new HashMap<>(); | ||
|
|
||
| static { | ||
| for (VeniceReadResponseStatus status: values()) { | ||
| CODE_MAP.put(status.code, status); | ||
| } |
There was a problem hiding this comment.
CODE_MAP is a mutable HashMap. Even though it’s private, accidental future mutation would break fromCode() determinism. Consider initializing into a local map and wrapping with Collections.unmodifiableMap(...) (or using Map.ofEntries(...)) to make the reverse-lookup table immutable.
1 participant
Summary
VeniceReadResponseStatusfrom a class withstatic final intconstants to a properenumwithint codefield,fromCode()reverse lookup, and new status codes (METHOD_NOT_ALLOWED,REQUEST_TIMEOUT,MISROUTED_STORE_VERSION)toByteString(byte[])andtoByteString(ByteBuf)zero-copy utility methods toGrpcUtilsHttpShortcutResponseto optionally carry aVeniceReadResponseStatus.getCode()where protointis expectedThis is PR 1 of a series to rewrite the gRPC read service on the Venice server, establishing common library foundations used by subsequent PRs.
Test plan
VeniceReadResponseStatusTestcoveringgetCode(),fromCode()round-trip, unknown/negative codes, uniquenessGrpcUtilsTestcases fortoByteString(byte array, ByteBuf, null/empty edge cases)./gradlew :internal:venice-common:buildpasses./gradlew :services:venice-server:buildpasses./gradlew :clients:venice-client:buildpasses./gradlew :clients:da-vinci-client:test --tests TestP2PFileTransferServerHandlerpasses