[Controller] Add gRPC support for getJobStatus API#2424
Closed
pthirun wants to merge 1 commit intolinkedin:mainfrom
Closed
[Controller] Add gRPC support for getJobStatus API#2424pthirun wants to merge 1 commit intolinkedin:mainfrom
pthirun wants to merge 1 commit intolinkedin:mainfrom
Conversation
Contributor
Author
|
/rerun-ci |
Add gRPC support for the getJobStatus endpoint while maintaining backward compatibility with the existing HTTP endpoint. Changes: - Add getJobStatus RPC to JobGrpcService.proto - Add handler method to JobRequestHandler - Add gRPC service implementation - Update HTTP route to convert requests to gRPC format - Add comprehensive unit and integration tests
46555e5 to
0d85a24
Compare
|
Hi there. This pull request has been inactive for 30 days. To keep our review queue healthy, we plan to close it in 7 days unless there is new activity. If you are still working on this, please push a commit, leave a comment, or convert it to draft to signal intent. Thank you for your time and contributions. |
|
Closing this pull request due to 37 days of inactivity. This is not a judgment on the value of the work. If you would like to continue, please reopen or open a new PR and we will be happy to take another look. Thank you again for contributing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem Statement
As part of the controller gRPC migration, the
getJobStatusAPI 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
getJobStatusAPI following the established migration pattern:JobGrpcService.proto): AddedgetJobStatusRPC method withGetJobStatusGrpcRequestandGetJobStatusGrpcResponsemessages, including support forUncompletedPartitionGrpcandUncompletedReplicaGrpcnested messagesJobRequestHandler.java): AddedgetJobStatus()method that validates parameters and callsadmin.getOffLinePushStatus(), returning a gRPC responseJobGrpcServiceImpl.java): AddedgetJobStatus()override (no ACL check needed for job status queries, matching HTTP endpoint behavior)JobRoutes.java): Updated to convert HTTP requests to gRPC format and delegate to the shared handlerAdminSparkServer.java): Updated to pass the request handler to the routeThe implementation reuses the existing business logic in
Admin.getOffLinePushStatus(), ensuring consistency between HTTP and gRPC endpoints.Code changes
Concurrency-Specific Checks
Both reviewer and PR author to verify
synchronized,RWLock) are used where needed. (N/A - no shared mutable state)ConcurrentHashMap,CopyOnWriteArrayList). (N/A - no collections introduced)handleRequest()utility for consistent error handling)How was this PR tested?
JobRequestHandlerTest: Tests for success, error, invalid input, missing parameters, and optional parametersJobGrpcServiceImplTest: Tests for success, error response, invalid argument error, started status, and optional parametersJobRoutesTest: Tests for success, error, started status, and gRPC-to-HTTP response mappingTestControllerGrpcEndpoints.testGetJobStatusGrpcEndpoint: End-to-end test that creates a store, starts an empty push, and queries job status via gRPC./gradlew :services:venice-controller:test :services:venice-controller:jacocoTestReportDoes this PR introduce any user-facing or breaking changes?