Feat: Support client-side interceptors #244
Open
Conversation
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
The Result[T] type replaces the non-generic, unexported response type. The reason to export the type is to make it easier to implement interceptors that need to use this type. Note that Result[T] is currently only used as Result[proto.Message] instances (in the current channel and as called from the existing calltypes (like quorumcall, multicast etc). In a future commit Result[T] will also be instansiated as Result[Resp], or Result[any], which can be used at the interceptor level.
This updates the testSrv implmentation to support the GetValue method. It also adds the echoSrv implmentation. These will be used for testing client-side interceptors.
Linters have been complaining about this style issue for a while.
Implement a flexible, composable interceptor architecture for quorum calls that provides better type safety, modularity, and extensibility compared to the legacy QuorumCall approach. Core Architecture: - QuorumInterceptor: Generic type for interceptor functions that wrap QuorumFunc - QuorumFunc: Function signature for processing quorum calls and returning results - ClientCtx: Context object providing access to request, config, and response iterator - Chain: Utility function to compose interceptors around a base handler Key Features: 1. Full type safety with generics (Req, Resp, Out type parameters) 2. Support for custom return types via Out parameter 3. Lazy message sending - transforms applied before dispatch 4. Iterator-based response handling with early termination support 5. Composable middleware pattern for building complex quorum logic Base Quorum Functions (Aggregators): - MajorityQuorum: Returns first response after ⌈(n+1)/2⌉ successful replies - FirstResponse: Returns first successful response (read-any pattern) - AllResponses: Waits for all nodes to respond (write-all pattern) - ThresholdQuorum: Generic threshold-based quorum with configurable count - CollectAllResponses: Returns map of all successful responses by node ID Interceptors (Middleware): - PerNodeTransform: Applies per-node request transformations with skip support - QuorumSpecInterceptor: Adapter for legacy QuorumSpec-style functions Iterator Helpers: - IgnoreErrors: Filters iterator to yield only successful responses - CollectN: Collects up to n responses into a map - CollectAll: Collects all responses into a map Implementation Details: - Lazy sending via sync.OnceFunc ensures transforms registered before dispatch - RegisterTransformFunc allows chaining multiple request transformations - applyTransforms applies registered transforms in order, skips invalid results - Responses() iterator yields node responses as they arrive - Type-safe conversion from Result[proto.Message] to Result[Resp] Backward Compatibility: - Legacy QuorumCall remains unchanged - QuorumSpecInterceptor bridges old and new approaches - No breaking changes to existing code Testing: - 17 comprehensive test functions covering unit and integration scenarios - Tests for iterator utilities, interceptor chaining, custom aggregation - Tests for per-node transformation with node skipping - Tests for all base quorum functions with various error conditions - Integration tests with real gRPC servers - Helper functions for consistent test patterns (testContext, checkError, etc.) This architecture enables gradual migration from the legacy approach and provides a foundation for future code generation template updates. Files changed: - client_interceptor.go (new, 446 lines) - client_interceptor_test.go (new, 794 lines)
Contributor
|
Here's the code health analysis summary for commits Analysis Summary
|
This fixes lint issues raised by deepsource and golangci-lint.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a flexible, composable interceptor architecture for quorum calls that provides significant improvements over the legacy QuorumCall approach through generic types, better type safety, and modular design.
Key changes:
- Implements generic interceptor types (QuorumInterceptor, QuorumFunc, ClientCtx) with lazy message sending via sync.OnceFunc
- Refactors response handling to use a generic Result[T] type instead of the old response struct
- Renames Incomplete error to ErrIncomplete following Go naming conventions
- Adds comprehensive test coverage with 17 test functions covering unit and integration scenarios
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| client_interceptor.go | Core interceptor infrastructure with generic types, base quorum functions (MajorityQuorum, FirstResponse, AllResponses, ThresholdQuorum, CollectAllResponses), interceptors (PerNodeTransform, QuorumSpecInterceptor), and iterator helpers |
| client_interceptor_test.go | Comprehensive test suite with 794 lines covering iterator utilities, interceptor chaining, custom aggregation, per-node transformations, and integration tests with real gRPC servers |
| channel.go | Refactored to use Result[T] generic type for responses and simplified response routing by storing request objects directly in responseRouters map |
| channel_test.go | Updated all test assertions to use Result[T] type with capitalized field names (NodeID, Value, Err) |
| rpc.go | Updated RPCCall to use Result[proto.Message] type for reply channel and field access |
| unicast.go | Updated Unicast to use Result[proto.Message] and embedded responseChan in request struct |
| multicast.go | Updated Multicast to use Result[proto.Message] type for reply channel |
| quorumcall.go | Updated QuorumCall to use Result[proto.Message] and renamed Incomplete to ErrIncomplete |
| async.go | Updated AsyncCall to use Result[proto.Message] type throughout |
| correctable.go | Updated CorrectableCall to use Result[proto.Message] and ErrIncomplete |
| errors.go | Renamed Incomplete to ErrIncomplete (following Go conventions) and added ErrTypeMismatch |
| errors_test.go | Updated tests to reference ErrIncomplete instead of Incomplete |
| testing_gorums.go | Enhanced test server implementation with value field for GetValue method testing and added echoServerFn helper |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This changes the iterator API from iter.Seq2[uint32, Result[T]] to a cleaner iter.Seq[Result[T]] pattern, and a type alias Results[T] which can serve as receiver for methods on said iterator. This simplifies the API by consolidating node ID and result information into a single Result[T] value, making iteration more ergonomic, despite not following Go's function-based iterator patterns. Key changes: - Introduce Results[T] type alias for iter.Seq[Result[T]] - Change ClientCtx.Responses() return type from iter.Seq2 to Results[T] - Update iterator helper methods to be methods on Results[T]: * IgnoreErrors() now returns Results[T] instead of iter.Seq2[uint32, T] * Add Filter() method for generic result filtering * CollectN() and CollectAll() now methods on Results[T] - Update all iterator consumers to use single-value iteration pattern - Constrain ClientCtx type parameters to msg (proto.Message) type Benefits: - Simpler iteration: `for result := range ctx.Responses()` vs `for nodeID, result := range ctx.Responses()` - More composable: method chaining like `ctx.Responses().IgnoreErrors().CollectAll()` - Consistent: Result[T] already contains NodeID, no need to pass separately - Cleaner: Filter() operates on complete Result[T] values This borrows from Asbjørn Salhus's design in PR #230, which I now agree is better than Go's function-based iterator pattern because of its significantly better composability. That is, you avoid composing with functions that would look like: gorums.IgnoreErrors(ctx.Responses())... and even worse when there are many iterators being composed.
Tests should fail with a deadline exceeded if they block; this is an indication of a deadlock issue that needs to be investigated.
Deepsource wants ctx to be the first argument, even in tests helpers.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This helps to more clearly distinguish the difference between an individual NodeResponse (previously Result) from the complete set of Responses (previously Results).
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.
This PR implements client-side interceptor logic to support an interceptor-based quorum call architecture. In a future update, I will update the
protoc-gen-gorumsgenerator to use the new interceptor architecture. This will significantly simplify our generator code, and allow us to remove several gorums options that will no longer be needed.Implement a flexible, composable interceptor architecture for quorum calls that provides better type safety, modularity, and extensibility compared to the legacy
QuorumCallapproach.Core Architecture:
QuorumInterceptor: Generic type for interceptor functions that wrapQuorumFuncQuorumFunc: Function signature for processing quorum calls and returning resultsClientCtx: Context object providing access to request, config, and response iteratorChain: Utility function to compose interceptors around a base handlerKey Features:
Base Quorum Functions (Aggregators):
MajorityQuorum: Returns first response after ⌈(n+1)/2⌉ successful repliesFirstResponse: Returns first successful response (read-any pattern)AllResponses: Waits for all nodes to respond (write-all pattern)ThresholdQuorum: Generic threshold-based quorum with configurable countCollectAllResponses: Returns a map of all successful responses by node IDInterceptors (Middleware):
PerNodeTransform: Applies per-node request transformations with skip supportQuorumSpecInterceptor: Adapter for legacy QuorumSpec-style functions (will be removed)Iterator Helpers:
IgnoreErrors: Filters the iterator to yield only successful responsesCollectN: Collects up to n responses into a mapCollectAll: Collects all responses into a mapImplementation Details:
Backward Compatibility:
The new architecture enables gradual migration from the legacy approach; however, the plan is to replace it wholesale. Hence, there is no plan to support legacy quorum call types.
Testing:
Updated API
After the initial design, I have updated the API to use composable iterator methods instead of the function-based variant promoted by the standard library. This was inspired by Asbjørn Salhus's implementation in #230, which was not designed for interceptors. The new design is now more composable.
Fixes #37