From f60e3a1b8fe8ff5d97a4cf97c513c9221190b8b0 Mon Sep 17 00:00:00 2001 From: pthirun Date: Wed, 28 Jan 2026 13:16:53 -0800 Subject: [PATCH] [Controller] Add gRPC support for deleteStore API Add gRPC support for the deleteStore endpoint while maintaining backward compatibility with the existing HTTP endpoint. Changes: - Add deleteStore RPC to StoreGrpcService.proto - Add handler method to StoreRequestHandler - Add gRPC service implementation - Update HTTP route to convert requests to gRPC format - Add comprehensive unit and integration tests --- .../proto/controller/StoreGrpcService.proto | 11 ++ .../endToEnd/TestControllerGrpcEndpoints.java | 101 ++++++++++++++++ .../grpc/server/StoreGrpcServiceImpl.java | 23 ++++ .../server/StoreRequestHandler.java | 42 +++++++ .../controller/server/StoresRoutes.java | 32 ++--- .../grpc/server/StoreGrpcServiceImplTest.java | 109 ++++++++++++++++++ .../server/StoreRequestHandlerTest.java | 85 ++++++++++++++ .../controller/server/StoresRoutesTest.java | 56 ++++++++- 8 files changed, 439 insertions(+), 20 deletions(-) diff --git a/internal/venice-common/src/main/proto/controller/StoreGrpcService.proto b/internal/venice-common/src/main/proto/controller/StoreGrpcService.proto index 82362064632..d2c4b78486d 100644 --- a/internal/venice-common/src/main/proto/controller/StoreGrpcService.proto +++ b/internal/venice-common/src/main/proto/controller/StoreGrpcService.proto @@ -14,6 +14,7 @@ service StoreGrpcService { rpc checkResourceCleanupForStoreCreation(ClusterStoreGrpcInfo) returns (ResourceCleanupCheckGrpcResponse) {} rpc validateStoreDeleted(ValidateStoreDeletedGrpcRequest) returns (ValidateStoreDeletedGrpcResponse); rpc listStores(ListStoresGrpcRequest) returns (ListStoresGrpcResponse); + rpc deleteStore(DeleteStoreGrpcRequest) returns (DeleteStoreGrpcResponse); } message CreateStoreGrpcRequest { @@ -82,4 +83,14 @@ message ListStoresGrpcRequest { message ListStoresGrpcResponse { string clusterName = 1; repeated string storeNames = 2; +} + +message DeleteStoreGrpcRequest { + ClusterStoreGrpcInfo storeInfo = 1; + optional bool isAbortMigrationCleanup = 2; +} + +message DeleteStoreGrpcResponse { + ClusterStoreGrpcInfo storeInfo = 1; + optional int64 executionId = 2; } \ No newline at end of file diff --git a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/endToEnd/TestControllerGrpcEndpoints.java b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/endToEnd/TestControllerGrpcEndpoints.java index 0218ed9b8c0..a7af70676e2 100644 --- a/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/endToEnd/TestControllerGrpcEndpoints.java +++ b/internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/endToEnd/TestControllerGrpcEndpoints.java @@ -17,6 +17,8 @@ import com.linkedin.venice.protocols.controller.ClusterStoreGrpcInfo; import com.linkedin.venice.protocols.controller.CreateStoreGrpcRequest; import com.linkedin.venice.protocols.controller.CreateStoreGrpcResponse; +import com.linkedin.venice.protocols.controller.DeleteStoreGrpcRequest; +import com.linkedin.venice.protocols.controller.DeleteStoreGrpcResponse; import com.linkedin.venice.protocols.controller.DiscoverClusterGrpcRequest; import com.linkedin.venice.protocols.controller.DiscoverClusterGrpcResponse; import com.linkedin.venice.protocols.controller.LeaderControllerGrpcRequest; @@ -249,6 +251,105 @@ public void testValidateStoreDeletedOverSecureGrpcChannel() { assertEquals(response.getStoreInfo().getClusterName(), veniceCluster.getClusterName()); } + @Test(timeOut = TIMEOUT_MS) + public void testDeleteStoreGrpcEndpoint() { + String storeName = Utils.getUniqueString("test_delete_store"); + String controllerGrpcUrl = veniceCluster.getLeaderVeniceController().getControllerGrpcUrl(); + ManagedChannel channel = Grpc.newChannelBuilder(controllerGrpcUrl, InsecureChannelCredentials.create()).build(); + StoreGrpcServiceGrpc.StoreGrpcServiceBlockingStub storeBlockingStub = StoreGrpcServiceGrpc.newBlockingStub(channel); + + ClusterStoreGrpcInfo storeGrpcInfo = ClusterStoreGrpcInfo.newBuilder() + .setClusterName(veniceCluster.getClusterName()) + .setStoreName(storeName) + .build(); + + // Step 1: Create the store + CreateStoreGrpcRequest createStoreGrpcRequest = CreateStoreGrpcRequest.newBuilder() + .setStoreInfo(storeGrpcInfo) + .setOwner("owner") + .setKeySchema(DEFAULT_KEY_SCHEMA) + .setValueSchema("\"string\"") + .build(); + CreateStoreGrpcResponse createResponse = storeBlockingStub.createStore(createStoreGrpcRequest); + assertNotNull(createResponse, "Response should not be null"); + assertEquals(createResponse.getStoreInfo().getStoreName(), storeName); + + // Step 2: Verify store exists + veniceCluster.useControllerClient(controllerClient -> { + StoreResponse storeResponse = TestUtils.assertCommand(controllerClient.getStore(storeName)); + assertNotNull(storeResponse.getStore(), "Store should exist after creation"); + }); + + // Step 3: Disable the store before deletion (required for deleteStore) + veniceCluster.useControllerClient(controllerClient -> { + TestUtils.assertCommand(controllerClient.enableStoreReadWrites(storeName, false)); + }); + + // Step 4: Delete the store via gRPC + DeleteStoreGrpcRequest deleteStoreRequest = DeleteStoreGrpcRequest.newBuilder().setStoreInfo(storeGrpcInfo).build(); + DeleteStoreGrpcResponse deleteResponse = storeBlockingStub.deleteStore(deleteStoreRequest); + assertNotNull(deleteResponse, "Response should not be null"); + assertEquals(deleteResponse.getStoreInfo().getClusterName(), veniceCluster.getClusterName()); + assertEquals(deleteResponse.getStoreInfo().getStoreName(), storeName); + + // Step 5: Verify store is deleted + TestUtils.waitForNonDeterministicAssertion(30, TimeUnit.SECONDS, () -> { + ValidateStoreDeletedGrpcRequest validateRequest = + ValidateStoreDeletedGrpcRequest.newBuilder().setStoreInfo(storeGrpcInfo).build(); + ValidateStoreDeletedGrpcResponse response = storeBlockingStub.validateStoreDeleted(validateRequest); + assertNotNull(response, "Response should not be null"); + assertTrue(response.getStoreDeleted(), "Store should be marked as deleted after deletion"); + }); + } + + @Test(timeOut = TIMEOUT_MS) + public void testDeleteStoreOverSecureGrpcChannel() { + String storeName = Utils.getUniqueString("test_delete_store_secure"); + String controllerSecureGrpcUrl = veniceCluster.getLeaderVeniceController().getControllerSecureGrpcUrl(); + ChannelCredentials credentials = GrpcUtils.buildChannelCredentials(sslFactory); + ManagedChannel channel = Grpc.newChannelBuilder(controllerSecureGrpcUrl, credentials).build(); + StoreGrpcServiceGrpc.StoreGrpcServiceBlockingStub storeBlockingStub = StoreGrpcServiceGrpc.newBlockingStub(channel); + + ClusterStoreGrpcInfo storeGrpcInfo = ClusterStoreGrpcInfo.newBuilder() + .setClusterName(veniceCluster.getClusterName()) + .setStoreName(storeName) + .build(); + + // First, create the store with an insecure channel (creation requires allowlist) + mockDynamicAccessController.addResourceToAllowList(storeName); + CreateStoreGrpcRequest createStoreGrpcRequest = CreateStoreGrpcRequest.newBuilder() + .setStoreInfo(storeGrpcInfo) + .setOwner("owner") + .setKeySchema(DEFAULT_KEY_SCHEMA) + .setValueSchema("\"string\"") + .build(); + CreateStoreGrpcResponse createResponse = storeBlockingStub.createStore(createStoreGrpcRequest); + assertNotNull(createResponse, "Response should not be null"); + + // Disable the store + veniceCluster.useControllerClient(controllerClient -> { + TestUtils.assertCommand(controllerClient.enableStoreReadWrites(storeName, false)); + }); + + DeleteStoreGrpcRequest deleteRequest = DeleteStoreGrpcRequest.newBuilder().setStoreInfo(storeGrpcInfo).build(); + + // Case 1: User not in allowlist for the resource - should get permission denied + mockDynamicAccessController.removeResourceFromAllowList(storeName); + assertFalse( + mockDynamicAccessController.isAllowlistUsers(null, storeName, Method.GET.name()), + "User should not be in allowlist"); + StatusRuntimeException exception = + Assert.expectThrows(StatusRuntimeException.class, () -> storeBlockingStub.deleteStore(deleteRequest)); + assertEquals(exception.getStatus().getCode(), io.grpc.Status.Code.PERMISSION_DENIED); + + // Case 2: User in allowlist - should succeed + mockDynamicAccessController.addResourceToAllowList(storeName); + DeleteStoreGrpcResponse deleteResponse = storeBlockingStub.deleteStore(deleteRequest); + assertNotNull(deleteResponse, "Response should not be null"); + assertEquals(deleteResponse.getStoreInfo().getStoreName(), storeName); + assertEquals(deleteResponse.getStoreInfo().getClusterName(), veniceCluster.getClusterName()); + } + @Test(timeOut = TIMEOUT_MS) public void testListStoresGrpcEndpoint() { String storeName1 = Utils.getUniqueString("test_list_stores_1"); diff --git a/services/venice-controller/src/main/java/com/linkedin/venice/controller/grpc/server/StoreGrpcServiceImpl.java b/services/venice-controller/src/main/java/com/linkedin/venice/controller/grpc/server/StoreGrpcServiceImpl.java index 1caddd66822..fedc24e2fac 100644 --- a/services/venice-controller/src/main/java/com/linkedin/venice/controller/grpc/server/StoreGrpcServiceImpl.java +++ b/services/venice-controller/src/main/java/com/linkedin/venice/controller/grpc/server/StoreGrpcServiceImpl.java @@ -12,6 +12,8 @@ import com.linkedin.venice.protocols.controller.CreateStoreGrpcResponse; import com.linkedin.venice.protocols.controller.DeleteAclForStoreGrpcRequest; import com.linkedin.venice.protocols.controller.DeleteAclForStoreGrpcResponse; +import com.linkedin.venice.protocols.controller.DeleteStoreGrpcRequest; +import com.linkedin.venice.protocols.controller.DeleteStoreGrpcResponse; import com.linkedin.venice.protocols.controller.GetAclForStoreGrpcRequest; import com.linkedin.venice.protocols.controller.GetAclForStoreGrpcResponse; import com.linkedin.venice.protocols.controller.ListStoresGrpcRequest; @@ -147,4 +149,25 @@ public void listStores(ListStoresGrpcRequest grpcRequest, StreamObserver responseObserver) { + LOGGER.debug("Received deleteStore with args: {}", grpcRequest); + String clusterName = grpcRequest.getStoreInfo().getClusterName(); + String storeName = grpcRequest.getStoreInfo().getStoreName(); + handleRequest(StoreGrpcServiceGrpc.getDeleteStoreMethod(), () -> { + if (!isAllowListUser(accessManager, storeName, Context.current())) { + throw new VeniceUnauthorizedAccessException( + ACL_CHECK_FAILURE_WARN_MESSAGE_PREFIX + StoreGrpcServiceGrpc.getDeleteStoreMethod().getFullMethodName() + + " on resource: " + storeName); + } + return storeRequestHandler.deleteStore(grpcRequest); + }, responseObserver, clusterName, storeName); + } } diff --git a/services/venice-controller/src/main/java/com/linkedin/venice/controller/server/StoreRequestHandler.java b/services/venice-controller/src/main/java/com/linkedin/venice/controller/server/StoreRequestHandler.java index f9eb6ee65d9..3fec5e60492 100644 --- a/services/venice-controller/src/main/java/com/linkedin/venice/controller/server/StoreRequestHandler.java +++ b/services/venice-controller/src/main/java/com/linkedin/venice/controller/server/StoreRequestHandler.java @@ -1,6 +1,7 @@ package com.linkedin.venice.controller.server; import com.linkedin.venice.controller.Admin; +import com.linkedin.venice.controller.AdminCommandExecutionTracker; import com.linkedin.venice.controller.ControllerRequestHandlerDependencies; import com.linkedin.venice.controller.StoreDeletedValidation; import com.linkedin.venice.exceptions.VeniceException; @@ -11,6 +12,8 @@ import com.linkedin.venice.protocols.controller.CreateStoreGrpcResponse; import com.linkedin.venice.protocols.controller.DeleteAclForStoreGrpcRequest; import com.linkedin.venice.protocols.controller.DeleteAclForStoreGrpcResponse; +import com.linkedin.venice.protocols.controller.DeleteStoreGrpcRequest; +import com.linkedin.venice.protocols.controller.DeleteStoreGrpcResponse; import com.linkedin.venice.protocols.controller.GetAclForStoreGrpcRequest; import com.linkedin.venice.protocols.controller.GetAclForStoreGrpcResponse; import com.linkedin.venice.protocols.controller.ListStoresGrpcRequest; @@ -255,4 +258,43 @@ public ListStoresGrpcResponse listStores(ListStoresGrpcRequest request) { LOGGER.info("Found {} stores in cluster: {}", selectedStoreNames.size(), clusterName); return ListStoresGrpcResponse.newBuilder().setClusterName(clusterName).addAllStoreNames(selectedStoreNames).build(); } + + /** + * Deletes a store from the specified Venice cluster. + * @param request the request containing cluster name, store name, and optional migration cleanup flag + * @return response with store info and optional execution ID for tracking + */ + public DeleteStoreGrpcResponse deleteStore(DeleteStoreGrpcRequest request) { + ClusterStoreGrpcInfo storeInfo = request.getStoreInfo(); + ControllerRequestParamValidator.validateClusterStoreInfo(storeInfo); + String clusterName = storeInfo.getClusterName(); + String storeName = storeInfo.getStoreName(); + boolean isAbortMigrationCleanup = request.hasIsAbortMigrationCleanup() && request.getIsAbortMigrationCleanup(); + + LOGGER.info( + "Deleting store: {} in cluster: {} with isAbortMigrationCleanup: {}", + storeName, + clusterName, + isAbortMigrationCleanup); + + DeleteStoreGrpcResponse.Builder responseBuilder = DeleteStoreGrpcResponse.newBuilder().setStoreInfo(storeInfo); + + Optional adminCommandExecutionTracker = + admin.getAdminCommandExecutionTracker(clusterName); + + if (adminCommandExecutionTracker.isPresent()) { + // Lock the tracker to get the execution id for the last admin command. + // It will not make our performance worse, because we lock the whole cluster while handling the admin + // operation in parent admin. + synchronized (adminCommandExecutionTracker) { + admin.deleteStore(clusterName, storeName, isAbortMigrationCleanup, Store.IGNORE_VERSION, false); + responseBuilder.setExecutionId(adminCommandExecutionTracker.get().getLastExecutionId()); + } + } else { + admin.deleteStore(clusterName, storeName, isAbortMigrationCleanup, Store.IGNORE_VERSION, false); + } + + LOGGER.info("Successfully deleted store: {} in cluster: {}", storeName, clusterName); + return responseBuilder.build(); + } } diff --git a/services/venice-controller/src/main/java/com/linkedin/venice/controller/server/StoresRoutes.java b/services/venice-controller/src/main/java/com/linkedin/venice/controller/server/StoresRoutes.java index abd65e910af..f70fc14847f 100644 --- a/services/venice-controller/src/main/java/com/linkedin/venice/controller/server/StoresRoutes.java +++ b/services/venice-controller/src/main/java/com/linkedin/venice/controller/server/StoresRoutes.java @@ -109,6 +109,8 @@ import com.linkedin.venice.meta.StoreInfo; import com.linkedin.venice.meta.Version; import com.linkedin.venice.protocols.controller.ClusterStoreGrpcInfo; +import com.linkedin.venice.protocols.controller.DeleteStoreGrpcRequest; +import com.linkedin.venice.protocols.controller.DeleteStoreGrpcResponse; import com.linkedin.venice.protocols.controller.ListStoresGrpcRequest; import com.linkedin.venice.protocols.controller.ListStoresGrpcResponse; import com.linkedin.venice.protocols.controller.ValidateStoreDeletedGrpcRequest; @@ -571,22 +573,24 @@ public void internalHandle(Request request, TrackableControllerResponse veniceRe String storeName = request.queryParams(NAME); boolean abortMigratingStore = Utils.parseBooleanOrFalse(request.queryParams(IS_ABORT_MIGRATION_CLEANUP), IS_ABORT_MIGRATION_CLEANUP); - veniceResponse.setCluster(clusterName); - veniceResponse.setName(storeName); - Optional adminCommandExecutionTracker = - admin.getAdminCommandExecutionTracker(clusterName); + // Build gRPC request from HTTP parameters + ClusterStoreGrpcInfo storeInfo = + ClusterStoreGrpcInfo.newBuilder().setClusterName(clusterName).setStoreName(storeName).build(); + DeleteStoreGrpcRequest.Builder grpcRequestBuilder = + DeleteStoreGrpcRequest.newBuilder().setStoreInfo(storeInfo); + if (abortMigratingStore) { + grpcRequestBuilder.setIsAbortMigrationCleanup(true); + } - if (adminCommandExecutionTracker.isPresent()) { - // Lock the tracker to get the execution id for the last admin command. - // It will not make our performance worse, because we lock the whole cluster while handling the admin - // operation in parent admin. - synchronized (adminCommandExecutionTracker) { - admin.deleteStore(clusterName, storeName, abortMigratingStore, Store.IGNORE_VERSION, false); - veniceResponse.setExecutionId(adminCommandExecutionTracker.get().getLastExecutionId()); - } - } else { - admin.deleteStore(clusterName, storeName, abortMigratingStore, Store.IGNORE_VERSION, false); + // Call handler + DeleteStoreGrpcResponse grpcResponse = storeRequestHandler.deleteStore(grpcRequestBuilder.build()); + + // Map response back to HTTP + veniceResponse.setCluster(grpcResponse.getStoreInfo().getClusterName()); + veniceResponse.setName(grpcResponse.getStoreInfo().getStoreName()); + if (grpcResponse.hasExecutionId()) { + veniceResponse.setExecutionId(grpcResponse.getExecutionId()); } } catch (Throwable e) { veniceResponse.setError(e); diff --git a/services/venice-controller/src/test/java/com/linkedin/venice/controller/grpc/server/StoreGrpcServiceImplTest.java b/services/venice-controller/src/test/java/com/linkedin/venice/controller/grpc/server/StoreGrpcServiceImplTest.java index 163bf13f345..e2b40a3d38c 100644 --- a/services/venice-controller/src/test/java/com/linkedin/venice/controller/grpc/server/StoreGrpcServiceImplTest.java +++ b/services/venice-controller/src/test/java/com/linkedin/venice/controller/grpc/server/StoreGrpcServiceImplTest.java @@ -23,6 +23,8 @@ import com.linkedin.venice.protocols.controller.CreateStoreGrpcResponse; import com.linkedin.venice.protocols.controller.DeleteAclForStoreGrpcRequest; import com.linkedin.venice.protocols.controller.DeleteAclForStoreGrpcResponse; +import com.linkedin.venice.protocols.controller.DeleteStoreGrpcRequest; +import com.linkedin.venice.protocols.controller.DeleteStoreGrpcResponse; import com.linkedin.venice.protocols.controller.GetAclForStoreGrpcRequest; import com.linkedin.venice.protocols.controller.GetAclForStoreGrpcResponse; import com.linkedin.venice.protocols.controller.ListStoresGrpcRequest; @@ -445,4 +447,111 @@ public void testListStoresWithFilters() { assertEquals(actualResponse.getClusterName(), TEST_CLUSTER, "Cluster name should match"); assertEquals(actualResponse.getStoreNamesCount(), 1, "Should have 1 store after filtering"); } + + @Test + public void testDeleteStoreReturnsSuccessfulResponse() { + when(controllerAccessManager.isAllowListUser(anyString(), any())).thenReturn(true); + ClusterStoreGrpcInfo storeInfo = + ClusterStoreGrpcInfo.newBuilder().setClusterName(TEST_CLUSTER).setStoreName(TEST_STORE).build(); + DeleteStoreGrpcRequest request = DeleteStoreGrpcRequest.newBuilder().setStoreInfo(storeInfo).build(); + DeleteStoreGrpcResponse response = + DeleteStoreGrpcResponse.newBuilder().setStoreInfo(storeInfo).setExecutionId(12345L).build(); + when(storeRequestHandler.deleteStore(any(DeleteStoreGrpcRequest.class))).thenReturn(response); + + DeleteStoreGrpcResponse actualResponse = blockingStub.deleteStore(request); + + assertNotNull(actualResponse, "Response should not be null"); + assertEquals(actualResponse.getStoreInfo(), storeInfo, "Store info should match"); + assertTrue(actualResponse.hasExecutionId(), "Should have execution ID"); + assertEquals(actualResponse.getExecutionId(), 12345L, "Execution ID should match"); + } + + @Test + public void testDeleteStoreReturnsSuccessfulResponseWithoutExecutionId() { + when(controllerAccessManager.isAllowListUser(anyString(), any())).thenReturn(true); + ClusterStoreGrpcInfo storeInfo = + ClusterStoreGrpcInfo.newBuilder().setClusterName(TEST_CLUSTER).setStoreName(TEST_STORE).build(); + DeleteStoreGrpcRequest request = DeleteStoreGrpcRequest.newBuilder().setStoreInfo(storeInfo).build(); + DeleteStoreGrpcResponse response = DeleteStoreGrpcResponse.newBuilder().setStoreInfo(storeInfo).build(); + when(storeRequestHandler.deleteStore(any(DeleteStoreGrpcRequest.class))).thenReturn(response); + + DeleteStoreGrpcResponse actualResponse = blockingStub.deleteStore(request); + + assertNotNull(actualResponse, "Response should not be null"); + assertEquals(actualResponse.getStoreInfo(), storeInfo, "Store info should match"); + assertFalse(actualResponse.hasExecutionId(), "Should not have execution ID"); + } + + @Test + public void testDeleteStoreWithAbortMigrationCleanup() { + when(controllerAccessManager.isAllowListUser(anyString(), any())).thenReturn(true); + ClusterStoreGrpcInfo storeInfo = + ClusterStoreGrpcInfo.newBuilder().setClusterName(TEST_CLUSTER).setStoreName(TEST_STORE).build(); + DeleteStoreGrpcRequest request = + DeleteStoreGrpcRequest.newBuilder().setStoreInfo(storeInfo).setIsAbortMigrationCleanup(true).build(); + DeleteStoreGrpcResponse response = DeleteStoreGrpcResponse.newBuilder().setStoreInfo(storeInfo).build(); + when(storeRequestHandler.deleteStore(any(DeleteStoreGrpcRequest.class))).thenReturn(response); + + DeleteStoreGrpcResponse actualResponse = blockingStub.deleteStore(request); + + assertNotNull(actualResponse, "Response should not be null"); + assertEquals(actualResponse.getStoreInfo(), storeInfo, "Store info should match"); + } + + @Test + public void testDeleteStoreReturnsErrorResponse() { + when(controllerAccessManager.isAllowListUser(anyString(), any())).thenReturn(true); + ClusterStoreGrpcInfo storeInfo = + ClusterStoreGrpcInfo.newBuilder().setClusterName(TEST_CLUSTER).setStoreName(TEST_STORE).build(); + DeleteStoreGrpcRequest request = DeleteStoreGrpcRequest.newBuilder().setStoreInfo(storeInfo).build(); + when(storeRequestHandler.deleteStore(any(DeleteStoreGrpcRequest.class))) + .thenThrow(new VeniceException("Failed to delete store")); + + StatusRuntimeException e = expectThrows(StatusRuntimeException.class, () -> blockingStub.deleteStore(request)); + + assertNotNull(e.getStatus(), "Status should not be null"); + assertEquals(e.getStatus().getCode(), Status.INTERNAL.getCode()); + VeniceControllerGrpcErrorInfo errorInfo = GrpcRequestResponseConverter.parseControllerGrpcError(e); + assertNotNull(errorInfo, "Error info should not be null"); + assertEquals(errorInfo.getErrorType(), ControllerGrpcErrorType.GENERAL_ERROR); + assertTrue(errorInfo.getErrorMessage().contains("Failed to delete store")); + } + + @Test + public void testDeleteStoreReturnsPermissionDenied() { + when(controllerAccessManager.isAllowListUser(anyString(), any())).thenReturn(false); + ClusterStoreGrpcInfo storeInfo = + ClusterStoreGrpcInfo.newBuilder().setClusterName(TEST_CLUSTER).setStoreName(TEST_STORE).build(); + DeleteStoreGrpcRequest request = DeleteStoreGrpcRequest.newBuilder().setStoreInfo(storeInfo).build(); + + StatusRuntimeException e = expectThrows(StatusRuntimeException.class, () -> blockingStub.deleteStore(request)); + + assertNotNull(e.getStatus(), "Status should not be null"); + assertEquals(e.getStatus().getCode(), Status.PERMISSION_DENIED.getCode()); + VeniceControllerGrpcErrorInfo errorInfo = GrpcRequestResponseConverter.parseControllerGrpcError(e); + assertNotNull(errorInfo, "Error info should not be null"); + assertEquals(errorInfo.getErrorType(), ControllerGrpcErrorType.UNAUTHORIZED); + assertTrue( + errorInfo.getErrorMessage().contains("Only admin users are allowed to run"), + "Actual: " + errorInfo.getErrorMessage()); + } + + @Test + public void testDeleteStoreReturnsBadRequestForInvalidArgument() { + when(controllerAccessManager.isAllowListUser(anyString(), any())).thenReturn(true); + ClusterStoreGrpcInfo storeInfo = + ClusterStoreGrpcInfo.newBuilder().setClusterName(TEST_CLUSTER).setStoreName(TEST_STORE).build(); + DeleteStoreGrpcRequest request = DeleteStoreGrpcRequest.newBuilder().setStoreInfo(storeInfo).build(); + when(storeRequestHandler.deleteStore(any(DeleteStoreGrpcRequest.class))) + .thenThrow(new IllegalArgumentException("Cluster name is mandatory parameter")); + + StatusRuntimeException e = expectThrows(StatusRuntimeException.class, () -> blockingStub.deleteStore(request)); + + assertNotNull(e.getStatus(), "Status should not be null"); + assertEquals(e.getStatus().getCode(), Status.INVALID_ARGUMENT.getCode()); + VeniceControllerGrpcErrorInfo errorInfo = GrpcRequestResponseConverter.parseControllerGrpcError(e); + assertNotNull(errorInfo, "Error info should not be null"); + assertEquals(errorInfo.getErrorType(), ControllerGrpcErrorType.BAD_REQUEST); + assertTrue(errorInfo.getErrorMessage().contains("Cluster name is mandatory parameter")); + } } diff --git a/services/venice-controller/src/test/java/com/linkedin/venice/controller/server/StoreRequestHandlerTest.java b/services/venice-controller/src/test/java/com/linkedin/venice/controller/server/StoreRequestHandlerTest.java index b0d87056ffe..5ec4d2bc2fa 100644 --- a/services/venice-controller/src/test/java/com/linkedin/venice/controller/server/StoreRequestHandlerTest.java +++ b/services/venice-controller/src/test/java/com/linkedin/venice/controller/server/StoreRequestHandlerTest.java @@ -10,6 +10,7 @@ import static org.testng.Assert.expectThrows; import com.linkedin.venice.controller.Admin; +import com.linkedin.venice.controller.AdminCommandExecutionTracker; import com.linkedin.venice.controller.ControllerRequestHandlerDependencies; import com.linkedin.venice.exceptions.VeniceException; import com.linkedin.venice.meta.DataReplicationPolicy; @@ -20,6 +21,8 @@ import com.linkedin.venice.protocols.controller.CreateStoreGrpcResponse; import com.linkedin.venice.protocols.controller.DeleteAclForStoreGrpcRequest; import com.linkedin.venice.protocols.controller.DeleteAclForStoreGrpcResponse; +import com.linkedin.venice.protocols.controller.DeleteStoreGrpcRequest; +import com.linkedin.venice.protocols.controller.DeleteStoreGrpcResponse; import com.linkedin.venice.protocols.controller.GetAclForStoreGrpcRequest; import com.linkedin.venice.protocols.controller.GetAclForStoreGrpcResponse; import com.linkedin.venice.protocols.controller.ListStoresGrpcRequest; @@ -358,4 +361,86 @@ public void testListStoresWithDataReplicationPolicyFilterNullPolicy() { assertEquals(response.getStoreNamesCount(), 0); } + + @Test + public void testDeleteStoreSuccess() { + DeleteStoreGrpcRequest request = DeleteStoreGrpcRequest.newBuilder() + .setStoreInfo(ClusterStoreGrpcInfo.newBuilder().setClusterName("testCluster").setStoreName("testStore").build()) + .build(); + + when(admin.getAdminCommandExecutionTracker("testCluster")).thenReturn(Optional.empty()); + + DeleteStoreGrpcResponse response = storeRequestHandler.deleteStore(request); + + verify(admin, times(1)).deleteStore("testCluster", "testStore", false, Store.IGNORE_VERSION, false); + assertEquals(response.getStoreInfo().getClusterName(), "testCluster"); + assertEquals(response.getStoreInfo().getStoreName(), "testStore"); + assertTrue(!response.hasExecutionId()); + } + + @Test + public void testDeleteStoreWithAbortMigrationCleanup() { + DeleteStoreGrpcRequest request = DeleteStoreGrpcRequest.newBuilder() + .setStoreInfo(ClusterStoreGrpcInfo.newBuilder().setClusterName("testCluster").setStoreName("testStore").build()) + .setIsAbortMigrationCleanup(true) + .build(); + + when(admin.getAdminCommandExecutionTracker("testCluster")).thenReturn(Optional.empty()); + + DeleteStoreGrpcResponse response = storeRequestHandler.deleteStore(request); + + verify(admin, times(1)).deleteStore("testCluster", "testStore", true, Store.IGNORE_VERSION, false); + assertEquals(response.getStoreInfo().getClusterName(), "testCluster"); + assertEquals(response.getStoreInfo().getStoreName(), "testStore"); + } + + @Test + public void testDeleteStoreWithExecutionTracker() { + DeleteStoreGrpcRequest request = DeleteStoreGrpcRequest.newBuilder() + .setStoreInfo(ClusterStoreGrpcInfo.newBuilder().setClusterName("testCluster").setStoreName("testStore").build()) + .build(); + + AdminCommandExecutionTracker tracker = mock(AdminCommandExecutionTracker.class); + when(tracker.getLastExecutionId()).thenReturn(12345L); + when(admin.getAdminCommandExecutionTracker("testCluster")).thenReturn(Optional.of(tracker)); + + DeleteStoreGrpcResponse response = storeRequestHandler.deleteStore(request); + + verify(admin, times(1)).deleteStore("testCluster", "testStore", false, Store.IGNORE_VERSION, false); + assertEquals(response.getStoreInfo().getClusterName(), "testCluster"); + assertEquals(response.getStoreInfo().getStoreName(), "testStore"); + assertTrue(response.hasExecutionId()); + assertEquals(response.getExecutionId(), 12345L); + } + + @Test(expectedExceptions = VeniceException.class) + public void testDeleteStoreThrowsException() { + DeleteStoreGrpcRequest request = DeleteStoreGrpcRequest.newBuilder() + .setStoreInfo(ClusterStoreGrpcInfo.newBuilder().setClusterName("testCluster").setStoreName("testStore").build()) + .build(); + + when(admin.getAdminCommandExecutionTracker("testCluster")).thenReturn(Optional.empty()); + doThrow(new VeniceException("Failed to delete store")).when(admin) + .deleteStore("testCluster", "testStore", false, Store.IGNORE_VERSION, false); + + storeRequestHandler.deleteStore(request); + } + + @Test(expectedExceptions = IllegalArgumentException.class) + public void testDeleteStoreMissingClusterName() { + DeleteStoreGrpcRequest request = DeleteStoreGrpcRequest.newBuilder() + .setStoreInfo(ClusterStoreGrpcInfo.newBuilder().setStoreName("testStore").build()) + .build(); + + storeRequestHandler.deleteStore(request); + } + + @Test(expectedExceptions = IllegalArgumentException.class) + public void testDeleteStoreMissingStoreName() { + DeleteStoreGrpcRequest request = DeleteStoreGrpcRequest.newBuilder() + .setStoreInfo(ClusterStoreGrpcInfo.newBuilder().setClusterName("testCluster").build()) + .build(); + + storeRequestHandler.deleteStore(request); + } } diff --git a/services/venice-controller/src/test/java/com/linkedin/venice/controller/server/StoresRoutesTest.java b/services/venice-controller/src/test/java/com/linkedin/venice/controller/server/StoresRoutesTest.java index c66d2adfad7..60fef3269f3 100644 --- a/services/venice-controller/src/test/java/com/linkedin/venice/controller/server/StoresRoutesTest.java +++ b/services/venice-controller/src/test/java/com/linkedin/venice/controller/server/StoresRoutesTest.java @@ -37,6 +37,8 @@ import com.linkedin.venice.meta.StoreInfo; import com.linkedin.venice.meta.ZKStore; import com.linkedin.venice.protocols.controller.ClusterStoreGrpcInfo; +import com.linkedin.venice.protocols.controller.DeleteStoreGrpcRequest; +import com.linkedin.venice.protocols.controller.DeleteStoreGrpcResponse; import com.linkedin.venice.protocols.controller.ListStoresGrpcRequest; import com.linkedin.venice.protocols.controller.ListStoresGrpcResponse; import com.linkedin.venice.protocols.controller.ValidateStoreDeletedGrpcRequest; @@ -116,26 +118,70 @@ public void testRollForwardToFutureVersion() throws Exception { @Test public void testDeleteStore() throws Exception { Admin mockAdmin = mock(VeniceParentHelixAdmin.class); + StoreRequestHandler mockRequestHandler = mock(StoreRequestHandler.class); doReturn(true).when(mockAdmin).isLeaderControllerFor(TEST_CLUSTER); Request request = mock(Request.class); doReturn(TEST_CLUSTER).when(request).queryParams(eq(ControllerApiConstants.CLUSTER)); doReturn(TEST_STORE_NAME).when(request).queryParams(eq(ControllerApiConstants.NAME)); + doReturn(null).when(request).queryParams(eq(ControllerApiConstants.IS_ABORT_MIGRATION_CLEANUP)); - Route deleteStoreRoute = new StoresRoutes(false, Optional.empty(), pubSubTopicRepository).deleteStore(mockAdmin); - TrackableControllerResponse trackableControllerResponse = ObjectMapperFactory.getInstance() + // Mock queryMap for error handling + QueryParamsMap queryParamsMap = mock(QueryParamsMap.class); + Map queryMap = new HashMap<>(2); + queryMap.put(ControllerApiConstants.CLUSTER, new String[] { TEST_CLUSTER }); + queryMap.put(ControllerApiConstants.NAME, new String[] { TEST_STORE_NAME }); + doReturn(queryMap).when(queryParamsMap).toMap(); + doReturn(queryParamsMap).when(request).queryMap(); + + ClusterStoreGrpcInfo storeInfo = + ClusterStoreGrpcInfo.newBuilder().setClusterName(TEST_CLUSTER).setStoreName(TEST_STORE_NAME).build(); + + // Case 1: Successful delete without execution ID + DeleteStoreGrpcResponse successResponse = DeleteStoreGrpcResponse.newBuilder().setStoreInfo(storeInfo).build(); + doReturn(successResponse).when(mockRequestHandler).deleteStore(any(DeleteStoreGrpcRequest.class)); + + Route deleteStoreRoute = + new StoresRoutes(false, Optional.empty(), pubSubTopicRepository, mockRequestHandler).deleteStore(mockAdmin); + String jsonResponse = deleteStoreRoute.handle(request, mock(Response.class)).toString(); + TrackableControllerResponse trackableControllerResponse = + ObjectMapperFactory.getInstance().readValue(jsonResponse, TrackableControllerResponse.class); + Assert.assertFalse( + trackableControllerResponse.isError(), + "Response should not have error. Error: " + trackableControllerResponse.getError()); + Assert.assertEquals(trackableControllerResponse.getCluster(), TEST_CLUSTER); + Assert.assertEquals(trackableControllerResponse.getName(), TEST_STORE_NAME); + + // Case 2: Successful delete with execution ID + DeleteStoreGrpcResponse responseWithExecutionId = + DeleteStoreGrpcResponse.newBuilder().setStoreInfo(storeInfo).setExecutionId(12345L).build(); + doReturn(responseWithExecutionId).when(mockRequestHandler).deleteStore(any(DeleteStoreGrpcRequest.class)); + + trackableControllerResponse = ObjectMapperFactory.getInstance() .readValue( deleteStoreRoute.handle(request, mock(Response.class)).toString(), TrackableControllerResponse.class); Assert.assertFalse(trackableControllerResponse.isError()); Assert.assertEquals(trackableControllerResponse.getCluster(), TEST_CLUSTER); Assert.assertEquals(trackableControllerResponse.getName(), TEST_STORE_NAME); + Assert.assertEquals(trackableControllerResponse.getExecutionId(), 12345L); + // Case 3: Delete with abort migration cleanup flag doReturn("true").when(request).queryParams(eq(ControllerApiConstants.IS_ABORT_MIGRATION_CLEANUP)); + doReturn(successResponse).when(mockRequestHandler).deleteStore(any(DeleteStoreGrpcRequest.class)); + + trackableControllerResponse = ObjectMapperFactory.getInstance() + .readValue( + deleteStoreRoute.handle(request, mock(Response.class)).toString(), + TrackableControllerResponse.class); + Assert.assertFalse(trackableControllerResponse.isError()); + + // Case 4: Handler throws exception with error type String errMessage = "Store " + TEST_STORE_NAME + "'s migrating flag is false. Not safe to delete a store " + "that is assumed to be migrating without the migrating flag setup as true."; - doThrow(new VeniceException(errMessage, INVALID_CONFIG)).when(mockAdmin) - .deleteStore(TEST_CLUSTER, TEST_STORE_NAME, true, Store.IGNORE_VERSION, false); + doThrow(new VeniceException(errMessage, INVALID_CONFIG)).when(mockRequestHandler) + .deleteStore(any(DeleteStoreGrpcRequest.class)); + trackableControllerResponse = ObjectMapperFactory.getInstance() .readValue( deleteStoreRoute.handle(request, mock(Response.class)).toString(), @@ -143,8 +189,6 @@ public void testDeleteStore() throws Exception { Assert.assertTrue(trackableControllerResponse.isError()); Assert.assertEquals(trackableControllerResponse.getErrorType(), INVALID_CONFIG); Assert.assertEquals(trackableControllerResponse.getError(), errMessage); - Assert.assertEquals(trackableControllerResponse.getCluster(), TEST_CLUSTER); - Assert.assertEquals(trackableControllerResponse.getName(), TEST_STORE_NAME); } @Test