[controller] Add support for gRPC in venice-controller - Part I #1396

sushantmane · 2024-12-16T13:33:36Z

Add support for gRPC in venice-controller - Part I

This commit introduces foundational server-side code to support gRPC-based endpoints in the Venice Controller. To keep the change set manageable and facilitate incremental progress, this PR includes only 3 out of the 15 endpoints implemented in #1244. The remaining endpoints will be added in the subsequent PRs.

How was this PR tested?

UTs and E2E tests

Does this PR introduce any user-facing changes?

No. You can skip the rest of this section.
Yes. Make sure to explain your proposed changes and call out the behavior change.

- Add controller handler service and gRPC server with secure mode - Implement create store, get store, getAllStores, list stores APIs - Add proto definitions for Stores and empty push - Enable configuration to toggle gRPC server on/off - Fix error handling and transport in ControllerClient for test compatibility - Add additional routes: AdminCommandExecution, AdminTopicMetadata, Cluster, Version - Update Docker setup for gRPC compatibility Add experimental changes Add integration test

- Add javadoc

FelixGV

Finished my first pass on this PR... left a few questions and comments. Thanks for the hard work on this! Exciting stuff!

FelixGV · 2025-01-08T02:18:00Z

internal/venice-common/src/main/java/com/linkedin/venice/grpc/VeniceGrpcServer.java

  }

  public boolean isTerminated() {
    return server.isTerminated();
  }

+  private boolean isSecure() {
+    return !(credentials instanceof InsecureServerCredentials);


Note that null instanceof Anything is always false. Right now, it doesn't seem like credentials can be null, but if they could be (e.g., due to some regression), then isSecure would return true (which would probably just result in a NPE sometime later, but in any event, it could be confusing?).

IDK what types can credentials take so IDK if a positive instanceof check is possible, or else maybe having a dedicated boolean flag for this, set in the same code path which sets the credentials prop. Or we can just leave it like this if you think it's fine. I don't have a strong opinion either way.

Here credentials can not be null as always set it to some value in the constructor of VeniceGrpcServer. But I agree this could be confusing hence I'v updated the code to use dedicated boolean flag.

FelixGV · 2025-01-08T02:19:27Z

internal/venice-common/src/main/proto/VeniceEchoService.proto

+  string methodType = 50001; // Assign a unique field number
+}
+
+service VeniceEchoService {


What is this for? Just some kind of hello world? Or a health check?

Ya, this is hello world kind of service that is only used for testing of VeniceGrpcServer class

All right, np, but can we move this class to the test directory then?

FelixGV · 2025-01-08T02:25:43Z

internal/venice-common/src/main/proto/VeniceStore.proto

+  map<string, ViewConfigGrpc> viewConfigs = 56;
+}
+
+message VersionGrpc {


I have a data modeling thought... it's not strictly gRPC-related, and also applies to the JSON stuff we shove into Zookeeper, but since we're building this one from scratch, we may have the option to reconsider some past decisions and do better (while leaving the JSON/ZK stuff unchanged -- at least for now)...

So the thing is that we have a lot of properties duplicated across store-version configs and store configs. And when that is the case, we implicitly understand it to mean that changing this config in the store config will affect future versions, and that we cannot change it in an existing version as those are immutable.

I wonder if we could codify this within the data model itself, and by the same token eliminate some duplication as well. How? By eliminating all of these props from the store config, and instead having a futureVersion record as one of the fields of the store config, which would house all of these configs.

IDK if the above idea is clear, please LMK either way. I'd like to know WDYT.

I like this idea a lot. I can update the PR to reflect this

FelixGV · 2025-01-08T02:29:04Z

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

-    createServices();
+    this.pubSubTopicRepository = new PubSubTopicRepository();
+    this.controllerService = createControllerService();
+    this.adminServer = createAdminServer(false);
+    this.secureAdminServer = sslEnabled ? createAdminServer(true) : null;
+    this.topicCleanupService = createTopicCleanupService();
+    this.storeBackupVersionCleanupService = createStoreBackupVersionCleanupService();
+    this.disabledPartitionEnablerService = createDisabledPartitionEnablerService();
+    this.unusedValueSchemaCleanupService = createUnusedValueSchemaCleanupService();
+    this.storeGraveyardCleanupService = createStoreGraveyardCleanupService();
+    this.systemStoreRepairService = createSystemStoreRepairService();


I like this kind of change. It should allow us to mark all/most of these properties final. Could you do that?

.../venice-controller/src/main/java/com/linkedin/venice/controller/server/ClusterDiscovery.java

FelixGV · 2025-01-08T02:31:30Z

.../venice-controller/src/main/java/com/linkedin/venice/controller/server/ControllerRoutes.java

-          // Controller is SSL Enabled
-          responseObject.setSecureUrl(leaderController.getUrl(true));
-        }
+        requestHandler.getLeaderController(new ControllerRequest(cluster), responseObject);


So this function will populate the responseObject? Can we not call it a getter if that is the case? e.g., populateLeaderController, perhaps?

FelixGV · 2025-01-08T02:42:21Z

...ler/src/main/java/com/linkedin/venice/controller/server/VeniceControllerGrpcServiceImpl.java

+  public void createStore(
+      CreateStoreGrpcRequest grpcRequest,
+      StreamObserver<CreateStoreGrpcResponse> responseObserver) {
+    String clusterName = grpcRequest.getClusterStoreInfo().getClusterName();
+    String storeName = grpcRequest.getClusterStoreInfo().getStoreName();
+    LOGGER.info("Received gRPC request to create store: {} in cluster: {}", storeName, clusterName);
+    try {
+      // TODO (sushantmane) : Add the ACL check for allowlist users here
+
+      // Convert the gRPC request to the internal request object
+      CreateNewStoreRequest request = new CreateNewStoreRequest(
+          grpcRequest.getClusterStoreInfo().getClusterName(),
+          grpcRequest.getClusterStoreInfo().getStoreName(),
+          grpcRequest.hasOwner() ? grpcRequest.getOwner() : null,
+          grpcRequest.getKeySchema(),
+          grpcRequest.getValueSchema(),
+          grpcRequest.hasAccessPermission() ? grpcRequest.getAccessPermission() : null,
+          grpcRequest.getIsSystemStore());
+
+      // Create the store using the internal request object
+      NewStoreResponse response = new NewStoreResponse();
+      requestHandler.createStore(request, response);
+
+      // Convert the internal response object to the gRPC response object and send the gRPC response
+      responseObserver.onNext(
+          CreateStoreGrpcResponse.newBuilder()
+              .setClusterStoreInfo(getClusterStoreGrpcInfo(response))
+              .setOwner(response.getOwner())
+              .build());
+      responseObserver.onCompleted();


I wonder if instead of having an intermediate POJO representation in the form of CreateNewStoreRequest and NewStoreResponse, could we use the gRPC generated classes, CreateStoreGrpcRequest and CreateStoreGrpcResponse , directly as our POJOs?

I think the intermediate representations can make sense if in the long-term we plan to not migrate fully to gRPC, and/or support additional protocols (i.e., besides gRPC and REST, some third one).

But since the long-term direction is to go all in on gRPC, I think it would be fine to use the gRPC classes as our basic POJOs, and do this kind of marshalling only in the REST bridging code (as a temporary stop gap, during the migration). Then, when we're done with the migration we could delete the REST code and the remainder would be very lean, no marshalling into separate objects, etc.

In a nutshell, this means:

requestHandler.createStore(request, response); would take the gRPC generated classes as param types.

This class here should be greatly simplified.

The REST class should be similar in size as in this PR, but transferring state into the gRPC POJOs instead of our own POJOs.

WDYT?

We discussed this during the AR, and Nisarg also suggested the same approach as you. However, my personal preference is to avoid mixing gRPC-generated classes throughout the codebase for two reasons:
1. If we decide to continue supporting HTTP/REST in the future, we would need to maintain HTTP ↔️ gRPC conversion logic
2. Using gRPC-generated classes directly in handlers would make unit testing more challenging since most of these generated classes are final

sushantmane force-pushed the grpc-in-venice-control-plane-basics branch 3 times, most recently from 052e0d8 to fd9b817 Compare December 16, 2024 19:22

sushantmane mentioned this pull request Dec 17, 2024

[controller] Client side foundations and refactor to support gRPC transport for controller APIs #1259

Open

2 tasks

sushantmane and others added 2 commits December 17, 2024 00:05

Add UTs for testLeaderControllerResponse

3450e01

- Add javadoc

sushantmane force-pushed the grpc-in-venice-control-plane-basics branch from fd9b817 to 405b106 Compare December 17, 2024 10:40

Refactor VeniceController

3c79933

sushantmane force-pushed the grpc-in-venice-control-plane-basics branch from 405b106 to 3c79933 Compare December 17, 2024 10:45

sushantmane requested a review from FelixGV December 18, 2024 05:26

FelixGV reviewed Jan 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[controller] Add support for gRPC in venice-controller - Part I #1396

[controller] Add support for gRPC in venice-controller - Part I #1396

sushantmane commented Dec 16, 2024 •

edited

Loading

FelixGV left a comment

FelixGV Jan 8, 2025

sushantmane Jan 8, 2025

FelixGV Jan 8, 2025

sushantmane Jan 8, 2025

FelixGV Jan 8, 2025

FelixGV Jan 8, 2025

sushantmane Jan 8, 2025

FelixGV Jan 8, 2025

FelixGV Jan 8, 2025

FelixGV Jan 8, 2025

sushantmane Jan 8, 2025

[controller] Add support for gRPC in venice-controller - Part I #1396

Are you sure you want to change the base?

[controller] Add support for gRPC in venice-controller - Part I #1396

Conversation

sushantmane commented Dec 16, 2024 • edited Loading

Add support for gRPC in venice-controller - Part I

How was this PR tested?

Does this PR introduce any user-facing changes?

FelixGV left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sushantmane commented Dec 16, 2024 •

edited

Loading