-
Notifications
You must be signed in to change notification settings - Fork 114
[common] Convert VeniceReadResponseStatus to enum and add GrpcUtils helpers #2462
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,7 @@ | ||
| package com.linkedin.venice.grpc; | ||
|
|
||
| import com.google.protobuf.ByteString; | ||
| import com.google.protobuf.UnsafeByteOperations; | ||
| import com.linkedin.venice.acl.handler.AccessResult; | ||
| import com.linkedin.venice.client.exceptions.VeniceClientException; | ||
| import com.linkedin.venice.exceptions.VeniceException; | ||
|
|
@@ -12,6 +14,7 @@ | |
| import io.grpc.ServerCall; | ||
| import io.grpc.Status; | ||
| import io.grpc.TlsChannelCredentials; | ||
| import io.netty.buffer.ByteBuf; | ||
| import java.io.IOException; | ||
| import java.io.InputStream; | ||
| import java.nio.file.Files; | ||
|
|
@@ -102,6 +105,24 @@ private static KeyStore loadStore(String path, char[] password, String type) | |
| return keyStore; | ||
| } | ||
|
|
||
| /** 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); | ||
| } | ||
|
|
||
| /** 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); | ||
|
Comment on lines
+116
to
+123
|
||
| } | ||
|
|
||
| public static ChannelCredentials buildChannelCredentials(SSLFactory sslFactory) { | ||
| // TODO: Evaluate if this needs to fail instead since it depends on plain text support on server | ||
| if (sslFactory == null) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,19 +1,41 @@ | ||
| package com.linkedin.venice.response; | ||
|
|
||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
|
|
||
| /** | ||
| * Enumeration of response status codes for Venice read requests. | ||
| * <p> | ||
| * **Positive values** correspond to standard HTTP status codes and can be used directly in HTTP responses. | ||
| * **Negative values** represent custom Venice-specific error codes. | ||
| * <p> | ||
| * For example, a status code of `200` indicates a successful read, while a status code of `-100` might indicate a specific Venice-related error. | ||
| * Positive values correspond to standard HTTP status codes and can be used directly in HTTP responses. | ||
| * Negative values represent custom Venice-specific error codes. | ||
| */ | ||
| public class VeniceReadResponseStatus { | ||
| public static final int KEY_NOT_FOUND = -420; | ||
|
|
||
| public static final int OK = 200; | ||
| public static final int BAD_REQUEST = 400; | ||
| public static final int INTERNAL_ERROR = 500; | ||
| public static final int TOO_MANY_REQUESTS = 429; | ||
| public static final int SERVICE_UNAVAILABLE = 503; | ||
| public enum VeniceReadResponseStatus { | ||
| KEY_NOT_FOUND(-420), OK(200), BAD_REQUEST(400), METHOD_NOT_ALLOWED(405), REQUEST_TIMEOUT(408), | ||
| MISROUTED_STORE_VERSION(410), TOO_MANY_REQUESTS(429), INTERNAL_ERROR(500), SERVICE_UNAVAILABLE(503); | ||
|
|
||
| private final int code; | ||
|
|
||
| private static final Map<Integer, VeniceReadResponseStatus> CODE_MAP = new HashMap<>(); | ||
|
|
||
| static { | ||
| for (VeniceReadResponseStatus status: values()) { | ||
| CODE_MAP.put(status.code, status); | ||
| } | ||
|
Comment on lines
+19
to
+24
|
||
| } | ||
|
|
||
| VeniceReadResponseStatus(int code) { | ||
| this.code = code; | ||
| } | ||
|
|
||
| public int getCode() { | ||
| return code; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the {@link VeniceReadResponseStatus} for the given integer code, or {@code null} if no match is found. | ||
| */ | ||
| public static VeniceReadResponseStatus fromCode(int code) { | ||
| return CODE_MAP.get(code); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| package com.linkedin.venice.response; | ||
|
|
||
| import static org.testng.Assert.assertEquals; | ||
| import static org.testng.Assert.assertNotNull; | ||
| import static org.testng.Assert.assertNull; | ||
|
|
||
| import java.util.HashSet; | ||
| import java.util.Set; | ||
| import org.testng.annotations.Test; | ||
|
|
||
|
|
||
| public class VeniceReadResponseStatusTest { | ||
| @Test | ||
| public void testGetCode() { | ||
| assertEquals(VeniceReadResponseStatus.KEY_NOT_FOUND.getCode(), -420); | ||
| assertEquals(VeniceReadResponseStatus.OK.getCode(), 200); | ||
| assertEquals(VeniceReadResponseStatus.BAD_REQUEST.getCode(), 400); | ||
| assertEquals(VeniceReadResponseStatus.METHOD_NOT_ALLOWED.getCode(), 405); | ||
| assertEquals(VeniceReadResponseStatus.REQUEST_TIMEOUT.getCode(), 408); | ||
| assertEquals(VeniceReadResponseStatus.MISROUTED_STORE_VERSION.getCode(), 410); | ||
| assertEquals(VeniceReadResponseStatus.TOO_MANY_REQUESTS.getCode(), 429); | ||
| assertEquals(VeniceReadResponseStatus.INTERNAL_ERROR.getCode(), 500); | ||
| assertEquals(VeniceReadResponseStatus.SERVICE_UNAVAILABLE.getCode(), 503); | ||
| } | ||
|
|
||
| @Test | ||
| public void testFromCode() { | ||
| for (VeniceReadResponseStatus status: VeniceReadResponseStatus.values()) { | ||
| VeniceReadResponseStatus resolved = VeniceReadResponseStatus.fromCode(status.getCode()); | ||
| assertNotNull(resolved, "fromCode should resolve " + status.name()); | ||
| assertEquals(resolved, status); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void testFromCodeWithUnknownCode() { | ||
| assertNull(VeniceReadResponseStatus.fromCode(999)); | ||
| assertNull(VeniceReadResponseStatus.fromCode(0)); | ||
| assertNull(VeniceReadResponseStatus.fromCode(-1)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testFromCodeWithNegativeCode() { | ||
| VeniceReadResponseStatus status = VeniceReadResponseStatus.fromCode(-420); | ||
| assertNotNull(status); | ||
| assertEquals(status, VeniceReadResponseStatus.KEY_NOT_FOUND); | ||
| } | ||
|
|
||
| @Test | ||
| public void testAllCodesAreUnique() { | ||
| Set<Integer> codes = new HashSet<>(); | ||
| for (VeniceReadResponseStatus status: VeniceReadResponseStatus.values()) { | ||
| boolean added = codes.add(status.getCode()); | ||
| assertEquals(added, true, "Duplicate code found: " + status.getCode()); | ||
| } | ||
| } | ||
| } |
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.
toByteString(byte[])usesUnsafeByteOperations.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).