feat(serverv2): integrate gRPC #21038
Conversation
WalkthroughWalkthroughThe changes present a significant refactor of gRPC method handling in the application, simplifying the interface by removing the need for byte slice inputs in decoding functions. Key methods and structures were renamed to reflect this streamlined approach, enhancing code clarity and maintainability while improving the functionality associated with gRPC interactions. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Server
participant Client
Client->>Server: gRPC Request
Server->>App: Retrieve Method Mapping
App-->>Server: Return Message Map
Server->>Server: Call Message Function
Server-->>Client: Send Response
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
# Conflicts: # server/v2/cometbft/server.go
|
@testinginprod your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files ignored due to path filters (1)
runtime/v2/go.sumis excluded by!**/*.sum
Files selected for processing (10)
- runtime/v2/app.go (2 hunks)
- runtime/v2/go.mod (5 hunks)
- runtime/v2/manager.go (4 hunks)
- runtime/v2/module.go (1 hunks)
- server/v2/api/grpc/gogoreflection/serverreflection.go (9 hunks)
- server/v2/api/grpc/server.go (5 hunks)
- server/v2/cometbft/abci.go (5 hunks)
- server/v2/cometbft/server.go (1 hunks)
- server/v2/go.mod (2 hunks)
- server/v2/types.go (1 hunks)
Files skipped from review due to trivial changes (1)
- runtime/v2/go.mod
Additional context used
Path-based instructions (8)
server/v2/types.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/app.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/api/grpc/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/module.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/api/grpc/gogoreflection/serverreflection.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/cometbft/abci.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.runtime/v2/manager.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (24)
server/v2/types.go (1)
20-20: LGTM! But verify the method usage in the codebase.The code changes are approved.
However, ensure that all implementations of
AppIinterface are updated to accommodate the new method signature.Verification successful
Verified: The method
GetGPRCMethodsToMessageMapis correctly implemented and used in the codebase.The method
GetGPRCMethodsToMessageMapis defined and used in the following files:
server/v2/types.goserver/v2/cometbft/server.goserver/v2/api/grpc/server.goruntime/v2/app.goAll occurrences reflect the new method signature and its intended usage.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all implementations of `AppI` interface have updated `GetGPRCMethodsToMessageMap` method. # Test: Search for the method usage. Expect: Only occurrences of the new method signature. rg --type go -A 5 $'GetGPRCMethodsToMessageMap'Length of output: 1113
runtime/v2/app.go (2)
47-49: LGTM!The renaming of
GRPCQueryDecoderstoGRPCMethodsToMessageMapsimplifies the interface and improves clarity.
121-122: LGTM! But verify the method usage in the codebase.The code changes are approved.
However, ensure that all calls to
GetGPRCMethodsToMessageMapmatch the new signature.server/v2/go.mod (1)
41-41: LGTM!The addition of
golang.org/x/expas a direct requirement improves clarity and control over the dependency versioning.server/v2/api/grpc/server.go (9)
26-29: LGTM! Constants for managing gRPC metadata and configuration options.The addition of
BlockHeightHeaderandFlagAddressconstants is a good practice for managing gRPC metadata and configuration options.
Line range hint
31-38:
LGTM! RenamingGRPCServertoServerimproves clarity.The renaming of
GRPCServertoServerenhances clarity and consistency. The struct fields are well-defined and necessary for the server's functionality.
40-44: LGTM!Newfunction correctly initializes theServerinstance.The
Newfunction is straightforward and correctly initializes theServerstruct with the provided configuration options.
Line range hint
48-74:
LGTM!Initfunction correctly initializes the gRPC server.The
Initfunction is well-structured and correctly initializes the gRPC server with the provided application interface, configuration, and logger. The new handler for unknown gRPC services enhances flexibility.
76-88: LGTM!StartCmdFlagsfunction facilitates runtime configuration.The
StartCmdFlagsfunction defines command-line flags specific to the gRPC server, facilitating its configuration at runtime.
90-128: LGTM!makeUnknownServiceHandlerenhances flexibility in managing gRPC calls.The
makeUnknownServiceHandlerfunction is well-designed and enhances the server's flexibility in managing gRPC calls by dynamically handling incoming requests based on the method name.
130-150: LGTM!getHeightFromCtxfunction handles block height metadata.The
getHeightFromCtxfunction is useful for retrieving and validating the block height from the incoming context, ensuring proper handling of requests that depend on this metadata.
Line range hint
170-195:
LGTM!Startfunction correctly starts the gRPC server.The
Startfunction is well-implemented and correctly handles the startup process of the gRPC server.
Line range hint
197-202:
LGTM!Stopfunction gracefully stops the gRPC server.The
Stopfunction is well-implemented and correctly handles the graceful shutdown of the gRPC server.runtime/v2/module.go (1)
138-138: LGTM!GRPCMethodsToMessageMapfield enhances gRPC method management.The addition of the
GRPCMethodsToMessageMapfield to theAppstruct provides a structured way to manage gRPC method-to-message associations, enhancing the application's ability to handle gRPC methods.server/v2/cometbft/server.go (1)
83-83: LGTM! UpdatedInitfunction enhances gRPC method handling.The modification in the
Initfunction to useappI.GetGPRCMethodsToMessageMap()instead ofappI.GetGRPCQueryDecoders()likely improves the way gRPC methods are handled or mapped within the server's initialization process, enhancing the server's capability to process gRPC-related tasks.server/v2/api/grpc/gogoreflection/serverreflection.go (4)
67-72: Enhancements toserverReflectionServerstruct look good.The addition of
methodsandlogfields enhances the functionality by allowing method tracking and structured logging.
76-80: Updates toRegisterfunction are appropriate.The new parameters
methodsandloggeralign with the enhancements to theserverReflectionServerstruct, enabling method tracking and structured logging.
96-98: Refactor ingetSymbolsmethod is well-implemented.The use of the new
getServicesmethod to retrieve services and file descriptors streamlines the process and enhances code clarity and maintainability.
480-502: Addition ofgetServicesmethod is a valuable enhancement.The method efficiently retrieves service names and file descriptors, processes method names, and includes proper error handling and logging.
server/v2/cometbft/abci.go (3)
63-63: Updates toConsensusstruct are beneficial.The removal of
grpcQueryDecodersand the addition ofgrpcMethodsMapsimplify gRPC handling by mapping method paths to message creator functions, improving clarity and efficiency.
Line range hint
73-82: Changes toNewConsensusconstructor are appropriate.The acceptance of
gRPCMethodsMapinstead ofgrpcQueryDecodersaligns with the updates to theConsensusstruct, ensuring consistency and improving the initialization process.
177-180: Updates toQuerymethod improve clarity and efficiency.The method now uses
grpcMethodsMapfor handling gRPC requests, calling a function from the map to create the appropriate message type, followed by unmarshalling the request data into that message.runtime/v2/manager.go (2)
582-582: Simplification ofgrpcQueryDecodersinconfiguratorstruct is beneficial.The map now returns a
gogoproto.Messagewithout any parameters, simplifying the decoder functions by removing the need to handle byte slices directly, likely improving performance and reducing complexity.
570-573: Refinement inregisterServicesfunction is well-implemented.The function now merges
grpcQueryDecodersintoapp.GRPCMethodsToMessageMap, suggesting a more refined approach to managing gRPC method mappings and improving the handling of message types.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for path, decoder := range c.grpcQueryDecoders { | ||
| app.GRPCMethodsToMessageMap[path] = decoder | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- server/v2/api/grpc/gogoreflection/serverreflection.go (8 hunks)
- server/v2/api/grpc/server.go (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- server/v2/api/grpc/server.go
Additional context used
Path-based instructions (1)
server/v2/api/grpc/gogoreflection/serverreflection.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (5)
server/v2/api/grpc/gogoreflection/serverreflection.go (5)
47-60: Imports look good.The new imports are necessary for the added functionalities.
67-72: Struct changes are appropriate.The addition of
methodsandlogfields enhances the server's ability to manage method descriptors and log relevant information.
76-80: Register function changes are valid.The additional parameters
methodsandloggerare necessary to initialize the new fields in theserverReflectionServerstruct.
95-97: Refactor in getSymbols method is effective.The use of the
getServicesmethod enhances clarity and maintainability.
210-213: Unmarshalling change is consistent.Switching to
gogoproto.Unmarshalensures consistency with the new library.
| func (s *serverReflectionServer) getServices(methods []string) (svcs []string, fds []*dpb.FileDescriptorProto) { | ||
| registry, err := gogoproto.MergedRegistry() | ||
| if err != nil { | ||
| s.log.Error("unable to load merged registry", "err", err) | ||
| return nil, nil | ||
| } | ||
| seenSvc := map[protoreflect.FullName]struct{}{} | ||
| for _, methodName := range methods { | ||
| methodName = strings.Join(strings.Split(methodName[1:], "/"), ".") | ||
| md, err := registry.FindDescriptorByName(protoreflect.FullName(methodName)) | ||
| if err != nil { | ||
| s.log.Error("unable to load method descriptor", "method", methodName, "err", err) | ||
| continue | ||
| } | ||
| svc := md.(protoreflect.MethodDescriptor).Parent() | ||
| if _, seen := seenSvc[svc.FullName()]; !seen { | ||
| svcs = append(svcs, string(svc.FullName())) | ||
| file := svc.ParentFile() | ||
| fds = append(fds, protodesc.ToFileDescriptorProto(file)) | ||
| } | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
Improve error handling and optimize getServices method.
The getServices method is well-structured but can be improved by handling errors more effectively and optimizing the conversion of method names.
- for _, methodName := range methods {
- methodName = strings.Join(strings.Split(methodName[1:], "/"), ".")
+ for _, method := range methods {
+ methodName := strings.ReplaceAll(method[1:], "/", ".")Additionally, consider logging the successful retrieval of method descriptors for better traceability.
+ s.log.Info("successfully retrieved method descriptor", "method", methodName)
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (2)
- server/v2/api/grpc/gogoreflection/serverreflection.go (8 hunks)
- server/v2/server_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- server/v2/api/grpc/gogoreflection/serverreflection.go
Additional context used
Path-based instructions (1)
server/v2/server_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
Additional comments not posted (1)
server/v2/server_test.go (1)
41-43: LGTM!The method correctly returns
nilfor a mock implementation.
| func (*mockApp[T]) GetGPRCMethodsToMessageMap() map[string]func() gogoproto.Message { | ||
| return map[string]func() gogoproto.Message{} | ||
| } |
There was a problem hiding this comment.
Correct the typo in the method name.
The method name should be GetGRPCMethodsToMessageMap instead of GetGPRCMethodsToMessageMap.
- func (*mockApp[T]) GetGPRCMethodsToMessageMap() map[string]func() gogoproto.Message {
+ func (*mockApp[T]) GetGRPCMethodsToMessageMap() map[string]func() gogoproto.Message {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (*mockApp[T]) GetGPRCMethodsToMessageMap() map[string]func() gogoproto.Message { | |
| return map[string]func() gogoproto.Message{} | |
| } | |
| func (*mockApp[T]) GetGRPCMethodsToMessageMap() map[string]func() gogoproto.Message { | |
| return map[string]func() gogoproto.Message{} | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Files selected for processing (4)
- schema/appdata/mux_test.go (1 hunks)
- server/v2/api/grpc/gogoreflection/serverreflection.go (9 hunks)
- server/v2/api/grpc/server.go (5 hunks)
- server/v2/server_test.go (2 hunks)
Files skipped from review due to trivial changes (1)
- schema/appdata/mux_test.go
Files skipped from review as they are similar to previous changes (1)
- server/v2/server_test.go
Additional context used
Path-based instructions (2)
server/v2/api/grpc/server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.server/v2/api/grpc/gogoreflection/serverreflection.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Additional comments not posted (16)
server/v2/api/grpc/server.go (8)
Line range hint
31-37:
LGTM! Struct definition is clear and well-defined.The
Serverstruct is appropriately defined with necessary fields.
40-44: LGTM! Function implementation is correct.The
Newfunction correctly initializes and returns a newServerinstance.
Line range hint
48-74:
LGTM! Function implementation is robust.The
Initfunction correctly initializes the gRPC server with configuration, methods mapping, and reflection.
76-88: LGTM! Function implementation is well-structured.The
StartCmdFlagsfunction correctly defines command-line flags for the gRPC server.
90-129: LGTM! Function implementation is robust.The
makeUnknownServiceHandlerfunction correctly handles unknown gRPC service requests with proper error handling and request-response cycle management.
131-151: LGTM! Function implementation is correct.The
getHeightFromCtxfunction correctly retrieves and validates the block height from the incoming context.
Line range hint
171-197:
LGTM! Function implementation is robust.The
Startfunction correctly starts the gRPC server with proper error handling and non-blocking behavior.
Line range hint
198-205:
LGTM! Function implementation is correct.The
Stopfunction correctly stops the gRPC server gracefully with proper logging.server/v2/api/grpc/gogoreflection/serverreflection.go (8)
59-72: LGTM! Struct definition is clear and well-defined.The
serverReflectionServerstruct is appropriately defined with new fields for methods and logging, enhancing functionality and logging capabilities.
75-80: LGTM! Function implementation is correct.The
Registerfunction correctly initializes theserverReflectionServerinstance with methods and logger parameters.
Line range hint
93-107:
LGTM! Function implementation is robust.The
getSymbolsfunction correctly retrieves service names and file descriptors using the newgetServicesmethod, improving clarity and maintainability.
Line range hint
108-126:
LGTM! Function implementation is correct.The
processFilefunction correctly processes file descriptors and their dependencies.
Line range hint
208-215:
LGTM! Function implementation is correct.The
decodeFileDescfunction correctly decodes and decompresses file descriptors with proper error handling.
Line range hint
273-290:
LGTM! Function implementation is correct.The
fileDescWithDependenciesfunction correctly returns file descriptors and their dependencies.
Line range hint
342-458:
LGTM! Function implementation is robust.The
ServerReflectionInfofunction correctly handles server reflection requests with proper handling of different types of reflection requests.
461-483: LGTM! Function implementation is correct.The
getServicesfunction correctly retrieves services based on method names with proper error handling and logging.
|
@mergify backport release/v0.52.x |
✅ Backports have been created
|
Co-authored-by: Marko <[email protected]> Co-authored-by: marbar3778 <[email protected]> (cherry picked from commit 8b47141) # Conflicts: # schema/appdata/mux_test.go # server/v2/cometbft/server.go # server/v2/go.mod
Co-authored-by: testinginprod <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
This PR Integrates gRPC in serverv2 leveraging
grpc.UnknownServiceHandlerto provide mapping between grpc methods and an appmanager query call.closes: #20798
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores