Skip to content

Commit 9b9bc35

Browse files
pthirunclaude
andcommitted
[Controller] Create separate SchemaGrpcService for schema operations
Extract getValueSchema from StoreGrpcService into a dedicated SchemaGrpcService for better separation of concerns. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 9617d4e commit 9b9bc35

8 files changed

Lines changed: 221 additions & 124 deletions

File tree

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
syntax = 'proto3';
2+
package com.linkedin.venice.protocols.controller;
3+
4+
5+
import "controller/ControllerGrpcRequestContext.proto";
6+
7+
option java_multiple_files = true;
8+
9+
service SchemaGrpcService {
10+
rpc getValueSchema(GetValueSchemaGrpcRequest) returns (GetValueSchemaGrpcResponse);
11+
}
12+
13+
message GetValueSchemaGrpcRequest {
14+
ClusterStoreGrpcInfo storeInfo = 1;
15+
int32 schemaId = 2;
16+
}
17+
18+
message GetValueSchemaGrpcResponse {
19+
ClusterStoreGrpcInfo storeInfo = 1;
20+
int32 schemaId = 2;
21+
string schemaStr = 3;
22+
}

internal/venice-common/src/main/proto/controller/StoreGrpcService.proto

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ service StoreGrpcService {
1414
rpc checkResourceCleanupForStoreCreation(ClusterStoreGrpcInfo) returns (ResourceCleanupCheckGrpcResponse) {}
1515
rpc validateStoreDeleted(ValidateStoreDeletedGrpcRequest) returns (ValidateStoreDeletedGrpcResponse);
1616
rpc listStores(ListStoresGrpcRequest) returns (ListStoresGrpcResponse);
17-
rpc getValueSchema(GetValueSchemaGrpcRequest) returns (GetValueSchemaGrpcResponse);
1817
}
1918

2019
message CreateStoreGrpcRequest {
@@ -83,15 +82,4 @@ message ListStoresGrpcRequest {
8382
message ListStoresGrpcResponse {
8483
string clusterName = 1;
8584
repeated string storeNames = 2;
86-
}
87-
88-
message GetValueSchemaGrpcRequest {
89-
ClusterStoreGrpcInfo storeInfo = 1;
90-
int32 schemaId = 2;
91-
}
92-
93-
message GetValueSchemaGrpcResponse {
94-
ClusterStoreGrpcInfo storeInfo = 1;
95-
int32 schemaId = 2;
96-
string schemaStr = 3;
9785
}

internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/endToEnd/TestControllerGrpcEndpoints.java

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import com.linkedin.venice.protocols.controller.LeaderControllerGrpcResponse;
2727
import com.linkedin.venice.protocols.controller.ListStoresGrpcRequest;
2828
import com.linkedin.venice.protocols.controller.ListStoresGrpcResponse;
29+
import com.linkedin.venice.protocols.controller.SchemaGrpcServiceGrpc;
2930
import com.linkedin.venice.protocols.controller.StoreGrpcServiceGrpc;
3031
import com.linkedin.venice.protocols.controller.StoreMigrationCheckGrpcRequest;
3132
import com.linkedin.venice.protocols.controller.StoreMigrationCheckGrpcResponse;
@@ -346,6 +347,8 @@ public void testGetValueSchemaGrpcEndpoint() {
346347
String controllerGrpcUrl = veniceCluster.getLeaderVeniceController().getControllerGrpcUrl();
347348
ManagedChannel channel = Grpc.newChannelBuilder(controllerGrpcUrl, InsecureChannelCredentials.create()).build();
348349
StoreGrpcServiceGrpc.StoreGrpcServiceBlockingStub storeBlockingStub = StoreGrpcServiceGrpc.newBlockingStub(channel);
350+
SchemaGrpcServiceGrpc.SchemaGrpcServiceBlockingStub schemaBlockingStub =
351+
SchemaGrpcServiceGrpc.newBlockingStub(channel);
349352

350353
ClusterStoreGrpcInfo storeGrpcInfo = ClusterStoreGrpcInfo.newBuilder()
351354
.setClusterName(veniceCluster.getClusterName())
@@ -367,7 +370,7 @@ public void testGetValueSchemaGrpcEndpoint() {
367370
GetValueSchemaGrpcRequest getSchemaRequest =
368371
GetValueSchemaGrpcRequest.newBuilder().setStoreInfo(storeGrpcInfo).setSchemaId(1).build();
369372

370-
GetValueSchemaGrpcResponse getSchemaResponse = storeBlockingStub.getValueSchema(getSchemaRequest);
373+
GetValueSchemaGrpcResponse getSchemaResponse = schemaBlockingStub.getValueSchema(getSchemaRequest);
371374
assertNotNull(getSchemaResponse, "Response should not be null");
372375
assertEquals(getSchemaResponse.getStoreInfo().getStoreName(), storeName);
373376
assertEquals(getSchemaResponse.getStoreInfo().getClusterName(), veniceCluster.getClusterName());
@@ -378,8 +381,8 @@ public void testGetValueSchemaGrpcEndpoint() {
378381
GetValueSchemaGrpcRequest invalidSchemaRequest =
379382
GetValueSchemaGrpcRequest.newBuilder().setStoreInfo(storeGrpcInfo).setSchemaId(99).build();
380383

381-
StatusRuntimeException exception =
382-
Assert.expectThrows(StatusRuntimeException.class, () -> storeBlockingStub.getValueSchema(invalidSchemaRequest));
384+
StatusRuntimeException exception = Assert
385+
.expectThrows(StatusRuntimeException.class, () -> schemaBlockingStub.getValueSchema(invalidSchemaRequest));
383386
assertEquals(exception.getStatus().getCode(), io.grpc.Status.Code.INVALID_ARGUMENT);
384387
}
385388

services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceController.java

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
import com.linkedin.venice.client.store.ClientConfig;
1010
import com.linkedin.venice.common.VeniceSystemStoreUtils;
1111
import com.linkedin.venice.controller.grpc.server.ClusterAdminOpsGrpcServiceImpl;
12+
import com.linkedin.venice.controller.grpc.server.SchemaGrpcServiceImpl;
1213
import com.linkedin.venice.controller.grpc.server.StoreGrpcServiceImpl;
1314
import com.linkedin.venice.controller.grpc.server.interceptor.ControllerGrpcAuditLoggingInterceptor;
1415
import com.linkedin.venice.controller.grpc.server.interceptor.ControllerGrpcSslSessionInterceptor;
@@ -329,8 +330,9 @@ private void initializeGrpcServer() {
329330
VeniceControllerGrpcServiceImpl grpcService = new VeniceControllerGrpcServiceImpl(unsecureRequestHandler);
330331
StoreGrpcServiceImpl storeGrpcServiceGrpc = new StoreGrpcServiceImpl(
331332
unsecureRequestHandler.getStoreRequestHandler(),
332-
unsecureRequestHandler.getSchemaRequestHandler(),
333333
unsecureRequestHandler.getControllerAccessManager());
334+
SchemaGrpcServiceImpl schemaGrpcService =
335+
new SchemaGrpcServiceImpl(unsecureRequestHandler.getSchemaRequestHandler());
334336
ClusterAdminOpsGrpcServiceImpl clusterAdminOpsGrpcService = new ClusterAdminOpsGrpcServiceImpl(
335337
unsecureRequestHandler.getClusterAdminOpsRequestHandler(),
336338
unsecureRequestHandler.getControllerAccessManager());
@@ -345,6 +347,7 @@ private void initializeGrpcServer() {
345347
new VeniceGrpcServerConfig.Builder().setPort(multiClusterConfigs.getAdminGrpcPort())
346348
.addService(grpcService)
347349
.addService(storeGrpcServiceGrpc)
350+
.addService(schemaGrpcService)
348351
.addService(clusterAdminOpsGrpcService)
349352
.setExecutor(grpcExecutor)
350353
.setInterceptors(interceptors)
@@ -358,15 +361,17 @@ private void initializeGrpcServer() {
358361
VeniceControllerGrpcServiceImpl secureGrpcService = new VeniceControllerGrpcServiceImpl(secureRequestHandler);
359362
StoreGrpcServiceImpl secureStoreGrpcService = new StoreGrpcServiceImpl(
360363
secureRequestHandler.getStoreRequestHandler(),
361-
secureRequestHandler.getSchemaRequestHandler(),
362364
secureRequestHandler.getControllerAccessManager());
365+
SchemaGrpcServiceImpl secureSchemaGrpcService =
366+
new SchemaGrpcServiceImpl(secureRequestHandler.getSchemaRequestHandler());
363367
ClusterAdminOpsGrpcServiceImpl secureClusterAdminOpsGrpcService = new ClusterAdminOpsGrpcServiceImpl(
364368
secureRequestHandler.getClusterAdminOpsRequestHandler(),
365369
secureRequestHandler.getControllerAccessManager());
366370
adminSecureGrpcServer = new VeniceGrpcServer(
367371
new VeniceGrpcServerConfig.Builder().setPort(multiClusterConfigs.getAdminSecureGrpcPort())
368372
.addService(secureGrpcService)
369373
.addService(secureStoreGrpcService)
374+
.addService(secureSchemaGrpcService)
370375
.addService(secureClusterAdminOpsGrpcService)
371376
.setExecutor(grpcExecutor)
372377
.setSslFactory(sslFactory)
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
package com.linkedin.venice.controller.grpc.server;
2+
3+
import com.linkedin.venice.controller.server.SchemaRequestHandler;
4+
import com.linkedin.venice.protocols.controller.GetValueSchemaGrpcRequest;
5+
import com.linkedin.venice.protocols.controller.GetValueSchemaGrpcResponse;
6+
import com.linkedin.venice.protocols.controller.SchemaGrpcServiceGrpc;
7+
import com.linkedin.venice.protocols.controller.SchemaGrpcServiceGrpc.SchemaGrpcServiceImplBase;
8+
import io.grpc.stub.StreamObserver;
9+
import org.apache.logging.log4j.LogManager;
10+
import org.apache.logging.log4j.Logger;
11+
12+
13+
public class SchemaGrpcServiceImpl extends SchemaGrpcServiceImplBase {
14+
private static final Logger LOGGER = LogManager.getLogger(SchemaGrpcServiceImpl.class);
15+
private final SchemaRequestHandler schemaRequestHandler;
16+
17+
public SchemaGrpcServiceImpl(SchemaRequestHandler schemaRequestHandler) {
18+
this.schemaRequestHandler = schemaRequestHandler;
19+
}
20+
21+
@Override
22+
public void getValueSchema(
23+
GetValueSchemaGrpcRequest request,
24+
StreamObserver<GetValueSchemaGrpcResponse> responseObserver) {
25+
LOGGER.debug("Received getValueSchema with args: {}", request);
26+
// No ACL check on getting store metadata (same as HTTP endpoint)
27+
ControllerGrpcServerUtils.handleRequest(
28+
SchemaGrpcServiceGrpc.getGetValueSchemaMethod(),
29+
() -> schemaRequestHandler.getValueSchema(request),
30+
responseObserver,
31+
request.getStoreInfo());
32+
}
33+
}

services/venice-controller/src/main/java/com/linkedin/venice/controller/grpc/server/StoreGrpcServiceImpl.java

Lines changed: 1 addition & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
import static com.linkedin.venice.controller.grpc.server.ControllerGrpcServerUtils.isAllowListUser;
55
import static com.linkedin.venice.controller.server.VeniceRouteHandler.ACL_CHECK_FAILURE_WARN_MESSAGE_PREFIX;
66

7-
import com.linkedin.venice.controller.server.SchemaRequestHandler;
87
import com.linkedin.venice.controller.server.StoreRequestHandler;
98
import com.linkedin.venice.controller.server.VeniceControllerAccessManager;
109
import com.linkedin.venice.exceptions.VeniceUnauthorizedAccessException;
@@ -15,8 +14,6 @@
1514
import com.linkedin.venice.protocols.controller.DeleteAclForStoreGrpcResponse;
1615
import com.linkedin.venice.protocols.controller.GetAclForStoreGrpcRequest;
1716
import com.linkedin.venice.protocols.controller.GetAclForStoreGrpcResponse;
18-
import com.linkedin.venice.protocols.controller.GetValueSchemaGrpcRequest;
19-
import com.linkedin.venice.protocols.controller.GetValueSchemaGrpcResponse;
2017
import com.linkedin.venice.protocols.controller.ListStoresGrpcRequest;
2118
import com.linkedin.venice.protocols.controller.ListStoresGrpcResponse;
2219
import com.linkedin.venice.protocols.controller.ResourceCleanupCheckGrpcResponse;
@@ -35,15 +32,10 @@
3532
public class StoreGrpcServiceImpl extends StoreGrpcServiceImplBase {
3633
private static final Logger LOGGER = LogManager.getLogger(StoreGrpcServiceImpl.class);
3734
private final StoreRequestHandler storeRequestHandler;
38-
private final SchemaRequestHandler schemaRequestHandler;
3935
private final VeniceControllerAccessManager accessManager;
4036

41-
public StoreGrpcServiceImpl(
42-
StoreRequestHandler storeRequestHandler,
43-
SchemaRequestHandler schemaRequestHandler,
44-
VeniceControllerAccessManager accessManager) {
37+
public StoreGrpcServiceImpl(StoreRequestHandler storeRequestHandler, VeniceControllerAccessManager accessManager) {
4538
this.storeRequestHandler = storeRequestHandler;
46-
this.schemaRequestHandler = schemaRequestHandler;
4739
this.accessManager = accessManager;
4840
}
4941

@@ -155,17 +147,4 @@ public void listStores(ListStoresGrpcRequest grpcRequest, StreamObserver<ListSto
155147
clusterName,
156148
null);
157149
}
158-
159-
@Override
160-
public void getValueSchema(
161-
GetValueSchemaGrpcRequest request,
162-
StreamObserver<GetValueSchemaGrpcResponse> responseObserver) {
163-
LOGGER.debug("Received getValueSchema with args: {}", request);
164-
// No ACL check on getting store metadata (same as HTTP endpoint)
165-
ControllerGrpcServerUtils.handleRequest(
166-
StoreGrpcServiceGrpc.getGetValueSchemaMethod(),
167-
() -> schemaRequestHandler.getValueSchema(request),
168-
responseObserver,
169-
request.getStoreInfo());
170-
}
171150
}
Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,151 @@
1+
package com.linkedin.venice.controller.grpc.server;
2+
3+
import static org.mockito.ArgumentMatchers.any;
4+
import static org.mockito.Mockito.mock;
5+
import static org.mockito.Mockito.when;
6+
import static org.testng.Assert.assertEquals;
7+
import static org.testng.Assert.assertNotNull;
8+
import static org.testng.Assert.assertTrue;
9+
import static org.testng.Assert.expectThrows;
10+
11+
import com.linkedin.venice.controller.grpc.GrpcRequestResponseConverter;
12+
import com.linkedin.venice.controller.server.SchemaRequestHandler;
13+
import com.linkedin.venice.exceptions.VeniceException;
14+
import com.linkedin.venice.protocols.controller.ClusterStoreGrpcInfo;
15+
import com.linkedin.venice.protocols.controller.ControllerGrpcErrorType;
16+
import com.linkedin.venice.protocols.controller.GetValueSchemaGrpcRequest;
17+
import com.linkedin.venice.protocols.controller.GetValueSchemaGrpcResponse;
18+
import com.linkedin.venice.protocols.controller.SchemaGrpcServiceGrpc;
19+
import com.linkedin.venice.protocols.controller.SchemaGrpcServiceGrpc.SchemaGrpcServiceBlockingStub;
20+
import com.linkedin.venice.protocols.controller.VeniceControllerGrpcErrorInfo;
21+
import io.grpc.ManagedChannel;
22+
import io.grpc.Server;
23+
import io.grpc.Status;
24+
import io.grpc.StatusRuntimeException;
25+
import io.grpc.inprocess.InProcessChannelBuilder;
26+
import io.grpc.inprocess.InProcessServerBuilder;
27+
import org.testng.annotations.AfterMethod;
28+
import org.testng.annotations.BeforeMethod;
29+
import org.testng.annotations.Test;
30+
31+
32+
public class SchemaGrpcServiceImplTest {
33+
private static final String TEST_CLUSTER = "test-cluster";
34+
private static final String TEST_STORE = "test-store";
35+
private static final String VALUE_SCHEMA = "string";
36+
37+
private Server grpcServer;
38+
private ManagedChannel grpcChannel;
39+
private SchemaRequestHandler schemaRequestHandler;
40+
private SchemaGrpcServiceBlockingStub blockingStub;
41+
42+
@BeforeMethod
43+
public void setUp() throws Exception {
44+
schemaRequestHandler = mock(SchemaRequestHandler.class);
45+
46+
// Create a unique server name for the in-process server
47+
String serverName = InProcessServerBuilder.generateName();
48+
49+
// Start the gRPC server in-process
50+
grpcServer = InProcessServerBuilder.forName(serverName)
51+
.directExecutor()
52+
.addService(new SchemaGrpcServiceImpl(schemaRequestHandler))
53+
.build()
54+
.start();
55+
56+
// Create a channel to communicate with the server
57+
grpcChannel = InProcessChannelBuilder.forName(serverName).directExecutor().build();
58+
59+
// Create a blocking stub to make calls to the server
60+
blockingStub = SchemaGrpcServiceGrpc.newBlockingStub(grpcChannel);
61+
}
62+
63+
@AfterMethod
64+
public void tearDown() throws Exception {
65+
if (grpcServer != null) {
66+
grpcServer.shutdown();
67+
}
68+
if (grpcChannel != null) {
69+
grpcChannel.shutdown();
70+
}
71+
}
72+
73+
@Test
74+
public void testGetValueSchemaReturnsSuccessfulResponse() {
75+
ClusterStoreGrpcInfo storeInfo =
76+
ClusterStoreGrpcInfo.newBuilder().setClusterName(TEST_CLUSTER).setStoreName(TEST_STORE).build();
77+
GetValueSchemaGrpcRequest request =
78+
GetValueSchemaGrpcRequest.newBuilder().setStoreInfo(storeInfo).setSchemaId(1).build();
79+
GetValueSchemaGrpcResponse expectedResponse = GetValueSchemaGrpcResponse.newBuilder()
80+
.setStoreInfo(storeInfo)
81+
.setSchemaId(1)
82+
.setSchemaStr(VALUE_SCHEMA)
83+
.build();
84+
when(schemaRequestHandler.getValueSchema(any(GetValueSchemaGrpcRequest.class))).thenReturn(expectedResponse);
85+
86+
GetValueSchemaGrpcResponse actualResponse = blockingStub.getValueSchema(request);
87+
88+
assertNotNull(actualResponse, "Response should not be null");
89+
assertEquals(actualResponse.getStoreInfo().getClusterName(), TEST_CLUSTER, "Cluster name should match");
90+
assertEquals(actualResponse.getStoreInfo().getStoreName(), TEST_STORE, "Store name should match");
91+
assertEquals(actualResponse.getSchemaId(), 1, "Schema ID should match");
92+
assertEquals(actualResponse.getSchemaStr(), VALUE_SCHEMA, "Schema string should match");
93+
}
94+
95+
@Test
96+
public void testGetValueSchemaReturnsErrorWhenSchemaNotFound() {
97+
ClusterStoreGrpcInfo storeInfo =
98+
ClusterStoreGrpcInfo.newBuilder().setClusterName(TEST_CLUSTER).setStoreName(TEST_STORE).build();
99+
GetValueSchemaGrpcRequest request =
100+
GetValueSchemaGrpcRequest.newBuilder().setStoreInfo(storeInfo).setSchemaId(99).build();
101+
when(schemaRequestHandler.getValueSchema(any(GetValueSchemaGrpcRequest.class)))
102+
.thenThrow(new IllegalArgumentException("Value schema for schema id: 99 of store: test-store doesn't exist"));
103+
104+
StatusRuntimeException e = expectThrows(StatusRuntimeException.class, () -> blockingStub.getValueSchema(request));
105+
106+
assertNotNull(e.getStatus(), "Status should not be null");
107+
assertEquals(e.getStatus().getCode(), Status.INVALID_ARGUMENT.getCode());
108+
VeniceControllerGrpcErrorInfo errorInfo = GrpcRequestResponseConverter.parseControllerGrpcError(e);
109+
assertNotNull(errorInfo, "Error info should not be null");
110+
assertEquals(errorInfo.getErrorType(), ControllerGrpcErrorType.BAD_REQUEST);
111+
assertTrue(errorInfo.getErrorMessage().contains("Value schema for schema id: 99"));
112+
}
113+
114+
@Test
115+
public void testGetValueSchemaReturnsErrorForInvalidInput() {
116+
ClusterStoreGrpcInfo storeInfo =
117+
ClusterStoreGrpcInfo.newBuilder().setClusterName("").setStoreName(TEST_STORE).build();
118+
GetValueSchemaGrpcRequest request =
119+
GetValueSchemaGrpcRequest.newBuilder().setStoreInfo(storeInfo).setSchemaId(1).build();
120+
when(schemaRequestHandler.getValueSchema(any(GetValueSchemaGrpcRequest.class)))
121+
.thenThrow(new IllegalArgumentException("Cluster name is mandatory parameter"));
122+
123+
StatusRuntimeException e = expectThrows(StatusRuntimeException.class, () -> blockingStub.getValueSchema(request));
124+
125+
assertNotNull(e.getStatus(), "Status should not be null");
126+
assertEquals(e.getStatus().getCode(), Status.INVALID_ARGUMENT.getCode());
127+
VeniceControllerGrpcErrorInfo errorInfo = GrpcRequestResponseConverter.parseControllerGrpcError(e);
128+
assertNotNull(errorInfo, "Error info should not be null");
129+
assertEquals(errorInfo.getErrorType(), ControllerGrpcErrorType.BAD_REQUEST);
130+
assertTrue(errorInfo.getErrorMessage().contains("Cluster name is mandatory parameter"));
131+
}
132+
133+
@Test
134+
public void testGetValueSchemaReturnsGeneralError() {
135+
ClusterStoreGrpcInfo storeInfo =
136+
ClusterStoreGrpcInfo.newBuilder().setClusterName(TEST_CLUSTER).setStoreName(TEST_STORE).build();
137+
GetValueSchemaGrpcRequest request =
138+
GetValueSchemaGrpcRequest.newBuilder().setStoreInfo(storeInfo).setSchemaId(1).build();
139+
when(schemaRequestHandler.getValueSchema(any(GetValueSchemaGrpcRequest.class)))
140+
.thenThrow(new VeniceException("Internal error fetching schema"));
141+
142+
StatusRuntimeException e = expectThrows(StatusRuntimeException.class, () -> blockingStub.getValueSchema(request));
143+
144+
assertNotNull(e.getStatus(), "Status should not be null");
145+
assertEquals(e.getStatus().getCode(), Status.INTERNAL.getCode());
146+
VeniceControllerGrpcErrorInfo errorInfo = GrpcRequestResponseConverter.parseControllerGrpcError(e);
147+
assertNotNull(errorInfo, "Error info should not be null");
148+
assertEquals(errorInfo.getErrorType(), ControllerGrpcErrorType.GENERAL_ERROR);
149+
assertTrue(errorInfo.getErrorMessage().contains("Internal error fetching schema"));
150+
}
151+
}

0 commit comments

Comments
 (0)