Conversation
Replace the Message wrapper with ordering.Metadata in the request struct and channel operations. This simplifies the wire protocol by using the generated protobuf types directly. Changes: - Replace `msg *Message` with `md *ordering.Metadata` in request struct - Update getStream() to return the typed Gorums_NodeStreamClient - Use stream.Send()/Recv() with Metadata instead of SendMsg()/RecvMsg() - Replace GetMessageID() with GetMessageSeqNo() for message routing - Add UnmarshalResponse() for deserializing response messages - Remove unused newRequestMessage() function from encoding.go
Replace legacy SendMsg/RecvMsg with type-safe Send/Recv methods in server.go, continuing the work from issue #252. Changes: - Change finished channel from chan *Message to chan *ordering.Metadata - Use srv.Recv() to receive *ordering.Metadata directly - Use srv.Send(md) to send metadata directly - Add UnmarshalRequest() to convert metadata to *Message for handlers - Add MarshalResponseMetadata() to convert *Message to metadata for sending - Update ServerCtx to use *ordering.Metadata channel internally - Clone metadata in MarshalResponseMetadata to avoid race conditions The Handler interface remains unchanged - conversion between *Message and *ordering.Metadata happens at the server boundaries, so no changes are needed to generated code.
Add responseWithError() helper function that creates a response message if needed and sets the error status in a single call. This simplifies the server's handler goroutine by: - Properly handling UnmarshalRequest errors (send error response to client) - Consolidating the nil message check and setError call into one line - Reducing the handler goroutine from 26 lines to 21 lines The helper is also useful for interceptors that need to return errors.
The custom Codec is no longer needed now that we use type-safe Send/Recv methods with *ordering.Metadata. gRPC's default proto codec handles serialization directly. We instead provide custom functions. Removed: - Codec struct and methods (Marshal, Unmarshal, gorumsMarshal, gorumsUnmarshal) - NewCodec() constructor - ContentSubtype constant - init() function that registered the codec - grpc.CallContentSubtype dial option - Test init() functions that registered the codec - mgr_test.go (only contained codec registration) This simplifies the codebase by removing an abstraction layer that was only needed for the legacy SendMsg/RecvMsg approach.
After removing the custom codec we no longer need the msgType field in the gorums.Message struct.
We had only one remaining use of NewRequest in a test; this removes NewRequest and replaces its use in TestAsProto with NewResponseMessage.
This replaces prior uses of NewGorumsMetadata.
Contributor
|
Here's the code health analysis summary for commits Analysis Summary
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors Gorums’ per-node stream communication to use gRPC’s type-safe streaming interfaces (Send/Recv) with ordering.Metadata, removing the legacy SendMsg/RecvMsg usage and eliminating the custom gRPC codec.
Changes:
- Switch client/server stream send/receive paths to typed
ordering.Metadata(Send/Recv) and route responses bymessage_seq_no. - Introduce request/response marshal/unmarshal helpers that encode the application proto into
Metadata.message_dataand decode viaprotoregistry. - Remove codec registration/content subtype wiring and update tests accordingly.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| unicast.go | Uses marshalRequest and enqueues metadata instead of *Message. |
| rpc.go | Uses marshalRequest and enqueues metadata for RPC calls. |
| channel.go | Reworks stream I/O to stream.Send(*Metadata) / stream.Recv() (*Metadata, error) and decodes responses from metadata. |
| server.go | Reworks server stream I/O to typed Recv/Send, introduces request decoding and response marshaling in SendMessage. |
| encoding.go | Removes codec implementation and adds (un)marshal helpers around Metadata.message_data + protoregistry. |
| ordering/gorums_metadata.go | Consolidates metadata construction into NewMetadata (now includes optional marshaled message bytes). |
| client_interceptor.go | Updates interceptor send path to clone metadata and set message_data per node/request. |
| channel_test.go | Updates tests/benchmarks to create/enqueue metadata-based requests. |
| encoding_test.go | Adjusts AsProto tests to reflect removal of NewRequest. |
| mgr.go | Removes default call content subtype setup (codec no longer used). |
| server_test.go / rpc_test.go / config_test.go | Removes codec registration from tests. |
| mgr_test.go | Removes manager logging test (deleted). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This commit refactors the internal messaging layer by replacing
the `ordering` package with `stream` and unifying the message
envelope structure.
Key changes:
- Rename the `ordering` package and proto files to `stream`.
- Update `stream.proto` to define a single `Message` type containing
both metadata and payload (as `bytes`), replacing the previous
split between `Metadata` and application message.
- Update `server.go` to:
- Use the new `stream` package.
- Implement explicit payload marshaling within `SendMessage`.
- Send error messages to the client on marshaling failures instead
of closing the stream.
- Update `encoding.go` to support packing/unpacking payloads into
`stream.Message`.
- Update `client_interceptor.go` to work with the new
`stream.Message` structure.
- Update `cmd/protoc-gen-gorums` templates to generate code compatible
with the new stream package and message format.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This is to avoid directly exposing the internal message types for the Gorums runtime (NodeStream).
This comment was marked as outdated.
This comment was marked as outdated.
- Remove expectedReplies field and always send one response per node (success, error, or skip) - Added ErrSkipNode for nodes skipped by request transformations - Update quorum and multicast logic to treat skips as non-errors - Simplify response iterator to use config size instead of expected count This fixes the issue where marshal failures or skips could cause the response iterator to stop early or block, ensuring exactly one response is sent per node in the configuration.
Don't reuse the in message as the response message as explained in a review comment; this can cause unnecessary bandwidth usage to send back the request payload to the client for no good reason. Review comment from Copilot: messageWithError reuses the incoming request message (in) when out is nil. That means error responses will echo the original request payload back to the client (wasted bandwidth, and surprising semantics). Prefer constructing a fresh response stream.Message (seqno+method+status, empty payload) or at least clear Payload/Entry when building an error-only response.
This resolves a code review complaint about incorrect gRPC-style path.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 41 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
responses.go:203
Responses.Thresholdcurrently treatsErrSkipNodeas a successful response (it does notcontinue), which means skipped nodes incrementcountand can setrespto a zero value. This can causeFirst()/Majority()/All()to return a nil/zero response withnilerror even though no node actually replied successfully. Skipped nodes should be ignored for success counting (e.g.,if errors.Is(result.Err, ErrSkipNode) { continue }) and onlycount++onresult.Err == nil.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This avoids the cmp.Or function here since it will always call the transformAndMarshal() method even if there is nothing to transform.
Member
Author
|
@copilot can you please revise the top-level pull request description to match the current state of the pull request. |
Contributor
meling
deleted the
meling/252/refactor-legacy-send-recv-stream-interface
branch
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.
Fixes #252
Summary of Changes: Type-safe stream communication
This PR introduces a significant refactor to simplify the internal communication layer and message handling. Key improvements include replacing the Message wrapper with direct Metadata usage, enforcing type-safe stream operations, and removing legacy codec implementations.
Refactoring & Simplification
Internal Restructuring
Maintenance