Skip to content

Commit 2ea66a5

Browse files
pthirunclaude
pthirun
and
claude
committed
Remove unused ControllerRequestContext and requestHandler parameter
Changes: - Remove ControllerRequestContext parameter from StoreRequestHandler.getRepushInfo() since no ACL check is required for this read-only operation - Remove StoreRequestHandler parameter from StoresRoutes.getRepushInfo() method and use the class member storeRequestHandler instead - Remove buildRequestContext() calls from both HTTP and gRPC layers - Remove private buildRequestContext() methods from StoresRoutes and StoreGrpcServiceImpl - Update AdminSparkServer to call getRepushInfo(admin) without requestHandler parameter - Update all test mocks to match new signatures This simplifies the code by removing unnecessary context building and parameter passing when no ACL check is needed. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1 parent 832f26d commit 2ea66a5

7 files changed

Lines changed: 16 additions & 57 deletions

File tree

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

Lines changed: 1 addition & 15 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.ControllerRequestContext;
87
import com.linkedin.venice.controller.server.StoreRequestHandler;
98
import com.linkedin.venice.controller.server.VeniceControllerAccessManager;
109
import com.linkedin.venice.controllerapi.RepushInfo;
@@ -174,11 +173,8 @@ public void getRepushInfo(
174173
String storeName = storeInfo.getStoreName();
175174
Optional<String> fabric = request.hasFabric() ? Optional.of(request.getFabric()) : Optional.empty();
176175

177-
// Build transport-agnostic context from gRPC
178-
ControllerRequestContext context = buildRequestContext(Context.current());
179-
180176
// Call handler - returns POJO
181-
RepushInfoResponse result = storeRequestHandler.getRepushInfo(clusterName, storeName, fabric, context);
177+
RepushInfoResponse result = storeRequestHandler.getRepushInfo(clusterName, storeName, fabric);
182178

183179
// Convert POJO to protobuf response
184180
RepushInfo repushInfo = result.getRepushInfo();
@@ -202,16 +198,6 @@ public void getRepushInfo(
202198
}, responseObserver, request.getStoreInfo());
203199
}
204200

205-
/**
206-
* Builds a ControllerRequestContext from the gRPC context.
207-
*/
208-
private ControllerRequestContext buildRequestContext(Context context) {
209-
GrpcControllerClientDetails clientDetails = ControllerGrpcServerUtils.getClientDetails(context);
210-
return new ControllerRequestContext(
211-
clientDetails.getClientCertificate(),
212-
clientDetails.getClientAddress() != null ? clientDetails.getClientAddress() : "anonymous");
213-
}
214-
215201
/**
216202
* Converts a Version object to protobuf VersionGrpc.
217203
*/

‎services/venice-controller/src/main/java/com/linkedin/venice/controller/server/AdminSparkServer.java‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -601,9 +601,7 @@ public boolean startInner() throws Exception {
601601
new VeniceParentControllerRegionStateHandler(admin, jobRoutes.getOngoingIncrementalPushVersions(admin)));
602602
httpService.get(
603603
GET_REPUSH_INFO.getPath(),
604-
new VeniceParentControllerRegionStateHandler(
605-
admin,
606-
storesRoutes.getRepushInfo(admin, requestHandler.getStoreRequestHandler())));
604+
new VeniceParentControllerRegionStateHandler(admin, storesRoutes.getRepushInfo(admin)));
607605
httpService.get(
608606
COMPARE_STORE.getPath(),
609607
new VeniceParentControllerRegionStateHandler(admin, storesRoutes.compareStore(admin)));

‎services/venice-controller/src/main/java/com/linkedin/venice/controller/server/StoreRequestHandler.java‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -264,14 +264,12 @@ public ListStoresGrpcResponse listStores(ListStoresGrpcRequest request) {
264264
* @param clusterName the cluster name
265265
* @param storeName the store name
266266
* @param fabric optional fabric for multi-region setups
267-
* @param context the request context (contains client identity)
268267
* @return RepushInfoResponse containing repush information including version and Kafka details
269268
*/
270269
public com.linkedin.venice.controllerapi.RepushInfoResponse getRepushInfo(
271270
String clusterName,
272271
String storeName,
273-
Optional<String> fabric,
274-
ControllerRequestContext context) {
272+
Optional<String> fabric) {
275273
LOGGER.info(
276274
"Getting repush info for store: {} in cluster: {} with fabric: {}",
277275
storeName,

‎services/venice-controller/src/main/java/com/linkedin/venice/controller/server/StoresRoutes.java‎

Lines changed: 2 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -252,7 +252,7 @@ public void internalHandle(Request request, SchemaUsageResponse response) {
252252
/**
253253
* @see Admin#getRepushInfo(String, String, Optional)
254254
*/
255-
public Route getRepushInfo(Admin admin, StoreRequestHandler requestHandler) {
255+
public Route getRepushInfo(Admin admin) {
256256
return new VeniceRouteHandler<RepushInfoResponse>(RepushInfoResponse.class) {
257257
@Override
258258
public void internalHandle(Request request, RepushInfoResponse veniceResponse) {
@@ -264,11 +264,8 @@ public void internalHandle(Request request, RepushInfoResponse veniceResponse) {
264264
String fabricName = request.queryParams(FABRIC);
265265
Optional<String> fabric = fabricName != null ? Optional.of(fabricName) : Optional.empty();
266266

267-
// Build transport-agnostic context
268-
ControllerRequestContext context = buildRequestContext(request);
269-
270267
// Call handler - returns POJO directly
271-
RepushInfoResponse result = requestHandler.getRepushInfo(clusterName, storeName, fabric, context);
268+
RepushInfoResponse result = storeRequestHandler.getRepushInfo(clusterName, storeName, fabric);
272269

273270
// Copy result to response
274271
veniceResponse.setCluster(result.getCluster());
@@ -1232,16 +1229,4 @@ public void internalHandle(Request request, StoreDeletedValidationResponse venic
12321229
}
12331230
};
12341231
}
1235-
1236-
/**
1237-
* Build request context from HTTP request.
1238-
*/
1239-
private ControllerRequestContext buildRequestContext(spark.Request request) {
1240-
if (!isSslEnabled()) {
1241-
return ControllerRequestContext.anonymous();
1242-
}
1243-
java.security.cert.X509Certificate cert = getCertificate(request);
1244-
String principalId = getPrincipalId(request);
1245-
return new ControllerRequestContext(cert, principalId);
1246-
}
12471232
}

‎services/venice-controller/src/test/java/com/linkedin/venice/controller/grpc/server/StoreGrpcServiceImplTest.java‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@
1414
import static org.testng.Assert.expectThrows;
1515

1616
import com.linkedin.venice.controller.grpc.GrpcRequestResponseConverter;
17-
import com.linkedin.venice.controller.server.ControllerRequestContext;
1817
import com.linkedin.venice.controller.server.StoreRequestHandler;
1918
import com.linkedin.venice.controller.server.VeniceControllerAccessManager;
2019
import com.linkedin.venice.controllerapi.RepushInfo;
@@ -475,8 +474,7 @@ public void testGetRepushInfoReturnsSuccessfulResponse() {
475474
mockResponse.setName(TEST_STORE);
476475
mockResponse.setRepushInfo(repushInfo);
477476

478-
when(storeRequestHandler.getRepushInfo(anyString(), anyString(), any(), any(ControllerRequestContext.class)))
479-
.thenReturn(mockResponse);
477+
when(storeRequestHandler.getRepushInfo(anyString(), anyString(), any())).thenReturn(mockResponse);
480478

481479
GetRepushInfoGrpcResponse actualResponse = blockingStub.getRepushInfo(request);
482480

@@ -494,7 +492,7 @@ public void testGetRepushInfoReturnsErrorResponse() {
494492
ClusterStoreGrpcInfo.newBuilder().setClusterName(TEST_CLUSTER).setStoreName(TEST_STORE).build();
495493
GetRepushInfoGrpcRequest request = GetRepushInfoGrpcRequest.newBuilder().setStoreInfo(storeInfo).build();
496494

497-
when(storeRequestHandler.getRepushInfo(anyString(), anyString(), any(), any(ControllerRequestContext.class)))
495+
when(storeRequestHandler.getRepushInfo(anyString(), anyString(), any()))
498496
.thenThrow(new VeniceException("Failed to get repush info"));
499497

500498
StatusRuntimeException e = expectThrows(StatusRuntimeException.class, () -> blockingStub.getRepushInfo(request));
@@ -515,8 +513,7 @@ public void testGetRepushInfoWithoutFabric() {
515513
mockResponse.setName(TEST_STORE);
516514
mockResponse.setRepushInfo(repushInfo);
517515

518-
when(storeRequestHandler.getRepushInfo(anyString(), anyString(), any(), any(ControllerRequestContext.class)))
519-
.thenReturn(mockResponse);
516+
when(storeRequestHandler.getRepushInfo(anyString(), anyString(), any())).thenReturn(mockResponse);
520517

521518
GetRepushInfoGrpcResponse actualResponse = blockingStub.getRepushInfo(request);
522519

‎services/venice-controller/src/test/java/com/linkedin/venice/controller/server/StoreRequestHandlerTest.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -383,7 +383,7 @@ public void testGetRepushInfoSuccess() {
383383

384384
when(admin.getRepushInfo(clusterName, storeName, fabric)).thenReturn(mockRepushInfo);
385385

386-
RepushInfoResponse response = storeRequestHandler.getRepushInfo(clusterName, storeName, fabric, context);
386+
RepushInfoResponse response = storeRequestHandler.getRepushInfo(clusterName, storeName, fabric);
387387

388388
verify(admin, times(1)).getRepushInfo(clusterName, storeName, fabric);
389389
assertEquals(response.getCluster(), clusterName);
@@ -419,7 +419,7 @@ public void testGetRepushInfoWithoutFabric() {
419419

420420
when(admin.getRepushInfo(clusterName, storeName, fabric)).thenReturn(mockRepushInfo);
421421

422-
RepushInfoResponse response = storeRequestHandler.getRepushInfo(clusterName, storeName, fabric, context);
422+
RepushInfoResponse response = storeRequestHandler.getRepushInfo(clusterName, storeName, fabric);
423423

424424
verify(admin, times(1)).getRepushInfo(clusterName, storeName, fabric);
425425
assertEquals(response.getCluster(), clusterName);
@@ -439,7 +439,7 @@ public void testGetRepushInfoWithNullVersion() {
439439

440440
when(admin.getRepushInfo(clusterName, storeName, fabric)).thenReturn(mockRepushInfo);
441441

442-
RepushInfoResponse response = storeRequestHandler.getRepushInfo(clusterName, storeName, fabric, context);
442+
RepushInfoResponse response = storeRequestHandler.getRepushInfo(clusterName, storeName, fabric);
443443

444444
assertEquals(response.getRepushInfo().getKafkaBrokerUrl(), "kafka.broker:9092");
445445
assertTrue(response.getRepushInfo().getVersion() == null);

‎services/venice-controller/src/test/java/com/linkedin/venice/controller/server/StoresRoutesTest.java‎

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -713,8 +713,7 @@ public void testGetRepushInfo() throws Exception {
713713
doReturn(queryMap).when(queryParamsMap).toMap();
714714
doReturn(queryParamsMap).when(request).queryMap();
715715

716-
Route route =
717-
new StoresRoutes(false, Optional.empty(), pubSubTopicRepository).getRepushInfo(mockAdmin, mockRequestHandler);
716+
Route route = new StoresRoutes(false, Optional.empty(), pubSubTopicRepository).getRepushInfo(mockAdmin);
718717

719718
// Create a real Version for admin.getRepushInfo() call to avoid Jackson serialization issues
720719
Version version = new VersionImpl(TEST_STORE_NAME, 1, "test-push-job", 10);
@@ -728,8 +727,7 @@ public void testGetRepushInfo() throws Exception {
728727
mockResponse.setName(TEST_STORE_NAME);
729728
mockResponse.setRepushInfo(mockRepushInfo);
730729

731-
when(mockRequestHandler.getRepushInfo(any(), any(), any(), any(ControllerRequestContext.class)))
732-
.thenReturn(mockResponse);
730+
when(mockRequestHandler.getRepushInfo(any(), any(), any())).thenReturn(mockResponse);
733731

734732
RepushInfoResponse response = ObjectMapperFactory.getInstance()
735733
.readValue(route.handle(request, mock(Response.class)).toString(), RepushInfoResponse.class);
@@ -741,8 +739,7 @@ public void testGetRepushInfo() throws Exception {
741739
Assert.assertEquals(response.getRepushInfo().getKafkaBrokerUrl(), "kafka.broker:9092");
742740

743741
// Test error case
744-
when(mockRequestHandler.getRepushInfo(any(), any(), any(), any(ControllerRequestContext.class)))
745-
.thenThrow(new VeniceException("Error"));
742+
when(mockRequestHandler.getRepushInfo(any(), any(), any())).thenThrow(new VeniceException("Error"));
746743
response = ObjectMapperFactory.getInstance()
747744
.readValue(route.handle(request, mock(Response.class)).toString(), RepushInfoResponse.class);
748745
Assert.assertTrue(response.isError());
@@ -767,8 +764,7 @@ public void testGetRepushInfoWithoutFabric() throws Exception {
767764
doReturn(queryMap).when(queryParamsMap).toMap();
768765
doReturn(queryParamsMap).when(request).queryMap();
769766

770-
Route route =
771-
new StoresRoutes(false, Optional.empty(), pubSubTopicRepository).getRepushInfo(mockAdmin, mockRequestHandler);
767+
Route route = new StoresRoutes(false, Optional.empty(), pubSubTopicRepository).getRepushInfo(mockAdmin);
772768

773769
RepushInfo mockRepushInfo = RepushInfo.createRepushInfo(null, "another.kafka:9092", null, null);
774770

@@ -777,8 +773,7 @@ public void testGetRepushInfoWithoutFabric() throws Exception {
777773
mockResponse.setName(TEST_STORE_NAME);
778774
mockResponse.setRepushInfo(mockRepushInfo);
779775

780-
when(mockRequestHandler.getRepushInfo(any(), any(), any(), any(ControllerRequestContext.class)))
781-
.thenReturn(mockResponse);
776+
when(mockRequestHandler.getRepushInfo(any(), any(), any())).thenReturn(mockResponse);
782777

783778
RepushInfoResponse response = ObjectMapperFactory.getInstance()
784779
.readValue(route.handle(request, mock(Response.class)).toString(), RepushInfoResponse.class);

0 commit comments

Comments
 (0)