Skip to content

[Controller] Add gRPC support for isStoreMigrationAllowed API - #2409

Merged
pthirun merged 6 commits into
linkedin:mainfrom
pthirun:controller-grpc-migration-is-store-migration-allowed
Jan 28, 2026
Merged

[Controller] Add gRPC support for isStoreMigrationAllowed API#2409
pthirun merged 6 commits into
linkedin:mainfrom
pthirun:controller-grpc-migration-is-store-migration-allowed

Conversation

@pthirun

@pthirun pthirun commented Jan 27, 2026

Copy link
Copy Markdown
Contributor

Problem Statement

As part of the controller gRPC migration, the isStoreMigrationAllowed API needs to be available via gRPC while maintaining the existing HTTP endpoint during the migration period. This follows the same pattern established by other controller API migrations, where business logic is abstracted so both HTTP and gRPC can share the same implementation.

Solution

Added gRPC support for the isStoreMigrationAllowed API following the established migration pattern:

  1. Proto definitions (ClusterAdminOpsGrpcService.proto): Added isStoreMigrationAllowed RPC method with IsStoreMigrationAllowedGrpcRequest and IsStoreMigrationAllowedGrpcResponse messages
  2. Request handler (ClusterAdminOpsRequestHandler.java): Added isStoreMigrationAllowed() method that validates parameters and calls admin.isStoreMigrationAllowed(), returning a gRPC response
  3. gRPC service implementation (ClusterAdminOpsGrpcServiceImpl.java): Added isStoreMigrationAllowed() override (no ACL check needed - any user is allowed to check migration status, matching HTTP endpoint behavior)
  4. HTTP route update (ClusterRoutes.java): Updated to convert HTTP requests to gRPC format and delegate to the shared handler
  5. Route registration (AdminSparkServer.java): Updated to pass the request handler to the route

The implementation reuses the existing business logic in Admin.isStoreMigrationAllowed(), ensuring consistency between HTTP and gRPC endpoints.

Code changes

  • Added new code behind a config. If so list the config names and their default values in the PR description.
  • Introduced new log lines.
    • Confirmed if logs need to be rate limited to avoid excessive logging. (Info level logging only, consistent with other gRPC endpoints)

Concurrency-Specific Checks

Both reviewer and PR author to verify

  • Code has no race conditions or thread safety issues. (Stateless request handling, delegates to existing thread-safe Admin methods)
  • Proper synchronization mechanisms (e.g., synchronized, RWLock) are used where needed. (N/A - no shared mutable state)
  • No blocking calls inside critical sections that could lead to deadlocks or performance degradation. (N/A - no critical sections)
  • Verified thread-safe collections are used (e.g., ConcurrentHashMap, CopyOnWriteArrayList). (N/A - no collections introduced)
  • Validated proper exception handling in multi-threaded code to avoid silent thread termination. (Uses handleRequest() utility for consistent error handling)

How was this PR tested?

  • New unit tests added.
    • Request Handler Tests (ClusterAdminOpsRequestHandlerTest.java):
      • testIsStoreMigrationAllowedReturnsTrue - Verifies handler returns true when migration is allowed
      • testIsStoreMigrationAllowedReturnsFalse - Verifies handler returns false when migration is not allowed
      • testIsStoreMigrationAllowedInvalidCluster - Verifies handler throws exception for invalid input
    • gRPC Service Tests (ClusterAdminOpsGrpcServiceImplTest.java):
      • testIsStoreMigrationAllowedSuccess - Verifies gRPC layer returns correct protobuf response
      • testIsStoreMigrationAllowedReturnsFalse - Verifies gRPC layer handles false value correctly
      • testIsStoreMigrationAllowedError - Verifies gRPC layer maps exceptions to INTERNAL status
    • HTTP Route Tests (ClusterRoutesTest.java):
      • testIsStoreMigrationAllowedReturnsTrue - Verifies HTTP layer converts to/from gRPC correctly
      • testIsStoreMigrationAllowedReturnsFalse - Verifies HTTP layer handles false value correctly
      • testIsStoreMigrationAllowedError - Verifies HTTP layer sets error flag on exception
  • New integration tests added.
    • Integration Test (TestControllerGrpcEndpoints.java):
      • testIsStoreMigrationAllowedGrpcEndpoint - End-to-end test with real Venice cluster
  • Modified or extended existing tests.
  • Verified backward compatibility (if applicable). (HTTP endpoint unchanged, gRPC is additive)
  • Unit test coverage verified: 100% instruction coverage for all new methods

Does this PR introduce any user-facing or breaking changes?

  • No. You can skip the rest of this section.
  • Yes. Clearly explain the behavior change and its impact.

@pthirun
pthirun force-pushed the controller-grpc-migration-is-store-migration-allowed branch from 3962d19 to fe60a04 Compare January 27, 2026 01:27
Add gRPC support for the isStoreMigrationAllowed endpoint while
maintaining backward compatibility with the existing HTTP endpoint.

Changes:
- Add isStoreMigrationAllowed RPC to ClusterAdminOpsGrpcService.proto
- Add handler method to ClusterAdminOpsRequestHandler
- Add gRPC service implementation in ClusterAdminOpsGrpcServiceImpl
- Update HTTP route to convert requests to gRPC format
- Update AdminSparkServer to pass request handler
- Add comprehensive unit tests for gRPC service and HTTP route

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@pthirun
pthirun force-pushed the controller-grpc-migration-is-store-migration-allowed branch from fe60a04 to 09c719b Compare January 27, 2026 01:32
Add missing unit tests for the isStoreMigrationAllowed method in
ClusterAdminOpsRequestHandler to achieve 100% code coverage.

Tests added:
- testIsStoreMigrationAllowedReturnsTrue
- testIsStoreMigrationAllowedReturnsFalse
- testIsStoreMigrationAllowedInvalidCluster

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@pthirun pthirun changed the title [Controller] Migrate isStoreMigrationAllowed API from HTTP to gRPC [Controller] Add gRPC support for isStoreMigrationAllowed API Jan 27, 2026

@sushantmane sushantmane left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM few minor comments

Comment thread internal/venice-common/src/main/proto/controller/ClusterAdminOpsGrpcService.proto Outdated
Comment thread internal/venice-common/src/main/proto/controller/ClusterAdminOpsGrpcService.proto Outdated
pthirun and others added 4 commits January 27, 2026 11:16
Rename IsStoreMigrationAllowedGrpcRequest/Response to
StoreMigrationCheckGrpcRequest/Response for brevity.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…d parameter

Update ClusterRoutes to accept ClusterAdminOpsRequestHandler in constructor
and use the field in isStoreMigrationAllowed, instead of accepting it as a
method parameter. This follows the cleaner pattern established in other routes.

Changes:
- ClusterRoutes: Add clusterAdminOpsRequestHandler field and constructor
- ClusterRoutes: Update isStoreMigrationAllowed(Admin, Handler) to isStoreMigrationAllowed(Admin)
- AdminSparkServer: Pass handler to ClusterRoutes constructor
- ClusterRoutesTest: Update tests to pass handler to constructor
Add testIsStoreMigrationAllowedGrpcEndpoint integration test to verify
the gRPC endpoint works end-to-end. This fixes the spotless check
failure caused by the unused assertTrue import.

@sushantmane sushantmane left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pthirun
pthirun merged commit 8109fcd into linkedin:main Jan 28, 2026
50 checks passed
@pthirun
pthirun deleted the controller-grpc-migration-is-store-migration-allowed branch January 28, 2026 23:09
sushantmane pushed a commit to sushantmane/venice that referenced this pull request Jan 30, 2026
misyel pushed a commit to misyel/venice that referenced this pull request Feb 2, 2026
sushantmane pushed a commit to sushantmane/venice that referenced this pull request Feb 8, 2026
misyel pushed a commit to misyel/venice that referenced this pull request Feb 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants